Allow null values to be treated as deselect options. #245
Conversation
|
It seems we have a failing test in master. I'll check that. |
|
The test that is failing is tabNavigation in ChosenIT. Could you fix the failing test or make sure that it still passes (depending on what is relevant)? |
|
On my local machine the test fails from master, I'm not sure if it is a version issue with Chrome/ChromeDriver, can you force a rebuild of master in team city to rule that out? |
|
Still failing after forcing the rebuild. |
|
What's the use case? I'm not sure I understand why you cannot use the current placeholder functionality? |
|
master fails on the same test http://teamcity.arcbees.com/viewLog.html?buildId=6239&tab=buildResultsDiv&buildTypeId=GwtChosen_MonitorPulls. The issue is that in order for allowSingleDeselect to actually be active the first item in the needs to have "" as its text, this is so there is an item to deselect to. When using ChosenValueListBox that means he renderer needs to render the first value as "", usually this is accomplished by inserting a null into the list and having the renderer render null as "". However, in the application I am developing the renderer renders null as "Select One". This breaks allowSingleDeselect because the text is no longer "", but is still the item to deselect to. This change makes it so that when a null value is passed into ChosenValueListBox it changes the value to "", but leaves the text the same as the render. Then I changed ChosenImpl to look at getValue(). Since getText() == getValue() when using the single argument addItem() this change does not break the old way that it worked. |
| } | ||
|
|
||
| /** | ||
| * Goal: ensure allowSingleDeselect shows the X/cross when there is a value |
There was a problem hiding this comment.
Checkstyle is complaining that your sentence does not end with a period. @meriouma I didn't know checkstyle was so picky! ;-)
|
It seems you are doing this pull request to bypass how the placeholder is handled. |
|
This is separate from a placeholder. With ChosenValueListBox one must add a null value to acceptable values in order to have something to deselect to. Therefore the first item in acceptableValues is null. Now, before my change, ChosenImpl would only select the null value if it rendered as "" (an empty string). In my application null is rendered as "Select One", while the merits of rendering null as "" or "Select One" can be debated, I won't touch on that subject. This change makes it so a null value has its form value as "", and ChosenImpl is updated to check the form value instead of the text of the rendered item. See for more details about null being in the list. |
|
Can I get some feedback on this? It is documented and fully tested, what else are you guys looking for? |
|
@jDramaix to review |
|
seems pretty straightforward. LGTM |
|
@awaters-escribe could you fix the conflicts? After that we could merge that PR as we have an LGTM from @jDramaix |
When a renderer doesn't render null as an empty string "" the setting allowSingleDeselect doesn't take effect. Therefore, when ChosenValueListBox encounters a null value it passes an empty string as the list box item's value. ChosenImpl now looks at the <option> element's value attribute to detect this, while keeping the old functionality operational.
bartmoc
force-pushed
the
master
branch
2 times, most recently
from
a34560c
to
e20e47b
Compare
control if the chosen value is non-null/non-empty.
when an option is selected/not-selected
ESH-3020 - Reverted wrong deselect control build check
When a renderer doesn't render null as an empty string "" the setting allowSingleDeselect doesn't take effect. Therefore, when ChosenValueListBox encounters a null value it passes an empty string as
the list box item's value. ChosenImpl now looks at the element's value attribute to detect this, while keeping the old functionality operational.