[JENKINS-72282] apply Jenkins styling to selects, radios and checkboxes #429
Conversation
There was a problem hiding this comment.
Thank you for the PR! Unfortunately looks like it cannot be merged as-is. Some of the functional tests fail to find elements in the UI with this change, probably due to the new DOM structure.
org.junit.runners.model.TestTimedOutException: test timed out after 180 seconds
at java.base/java.lang.Thread.sleep0(Native Method)
at java.base/java.lang.Thread.sleep(Thread.java:509)
at org.openqa.selenium.support.ui.Sleeper.lambda$static$0(Sleeper.java:25)
at org.openqa.selenium.support.ui.FluentWait.until(FluentWait.java:232)
at org.biouno.unochoice.BaseUiTest.checkOptions(BaseUiTest.java:148)
at org.biouno.unochoice.UiAcceptanceTest.test(UiAcceptanceTest.java:84)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:659)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
at java.base/java.lang.Thread.run(Thread.java:1583)We'd have to adjust the JS code at least (not sure if anything in Java or jelly would need to be changed), so that the tests pass, and then do a few manual tests before merging this.
|
Or.... maybe it'd be possible to use css/scss to copy the jenkins styles to the current DOM layout. Can't tell if that'd be a good idea, nor easy to implement, but that's an alternative, I think. |
set classes on the td div instead of adding additional div/span
|
I have now adjusted the js code so it also renders the elements properly, at least for CascadeParameter. Tests are green now. |
| @@ -658,6 +665,10 @@ var UnoChoice = UnoChoice || (jQuery3 => { | |||
| let idValue = `ecp_${e.target.randomName}_${i}`; | |||
| idValue = idValue.replace(' ', '_'); | |||
|
|
|||
| let tdClass = | |||
There was a problem hiding this comment.
Not sure if this is correct.
As the if from line 662 applies this to checkboxes I have the impression that a lot of the code here might be obsolete.
Can the variable entry ever be something else than an input of type checkbox here? If yes how do I need to setup the parameters to achieve this, e.g. how to make entry a string or a JSON?
| @@ -694,14 +705,15 @@ var UnoChoice = UnoChoice || (jQuery3 => { | |||
| input.checked = false; | |||
| let jsonInput = util.makeHidden(input.getAttribute('otherid'), input.getAttribute('json'), '', input.getAttribute('value'), input.getAttribute('name'), input.getAttribute('alt')); | |||
|
|
|||
| let label = util.makeLabel(input.getAttribute('alt'), undefined); | |||
| let label = util.makeLabel(input.getAttribute('alt'), undefined, "jenkins-radio__label"); | |||
There was a problem hiding this comment.
more or less the same question: Can entry here ever be something else than an input of type radio
PS: line 703 is obsolete as input is replaced by entry in line 704
mawinter69
changed the title
There was a problem hiding this comment.
Hi @mawinter69 !
I had another look at the code today, and didn't find anything that seemed wrong at first sight. I'll have a look later, but first I wanted to confirm if you have more changes planned (I saw your latest changes were mainly to fix the tests?), and to also let you know that I'll merge this PR not for the next release, but for the subsequent one.
We had a regression in some of the latest versions, and I'm planning to release with what we have (mainly code migrated to match Jenkins API updates, and dependabots, and also the fix for the regression).
This way, it should be easier to confirm if the regression is fixed, without having to worry if this PR or other PRs may have accidentally caused another regression.
Thanks!
|
sure, no problem to wait with this one. From my side everything is fine. I had some questions though where if my assumption is wrong some changes might be required. |
The change will make active choice and active reactive choice parameters take the current Jenkins stylings.
New look:
additional changes:
Testing done
Interactive testing
Submitter checklist