Fix show selected on top for combobox #84
Conversation
…f items), do not treat fake created items as real
| })) | ||
| .sortBy((o) => { | ||
| // Fake entries go to the bottom and selected entries go to the top only if alwaysShowSelectedOnTop was passed | ||
| if (o.isFake) { |
There was a problem hiding this comment.
This is going to start to get messy. What happens when someone else wants one of the fake options to be at the top? Would it be better to have isFake and listLast (or maybe a better name)?
There was a problem hiding this comment.
Another thought... what if we use null for the value instead of an isFake property?
There was a problem hiding this comment.
. what if we use null for the value instead of an isFake property?
I can try that but it wouldn't help for the sorting thing. Also, we would have the same problem if we ever pass null as a valid value for the option.
What happens when someone else wants one of the fake options to be at the top?
It's funny that you ask that because as soon as I sent this PR, I saw this... so if we want to do that (and not have the change apply to all selectors) then we need a new prop passed to the component.
Would it be better to have isFake and listLast (or maybe a better name)?
What do you mean?
There was a problem hiding this comment.
I can try that but it wouldn't help for the sorting thing
Yeah, I know that doesn't help for the sorting, but I'm trying to separate the two (I don't think sorting should be based off of anything except something related to sorting).
What do you mean?
Use listLast to solve point 1 from the PR description and use isFake (or null value) to solve point 2.
|
Updated |
@tgolen , will you please review this?
There were 2 issues:
showSelectedOnTopwas not working because we got the first X before sorting them.''the component thought we were selecting theViewing first ${this.props.maxItemsToShow} items. Search to see more.because we created that option with value of''.Needed for
https://github.com/Expensify/Expensify/issues/73200#event-1471060712
Tests
Viewing first...text is not shown in the combobox as selected.