ref(compactSelect): improve clearing values #103720
Conversation
and fix types to make onChange emit `| undefined` when `clearable` is set
github-actions
bot
added
the
Scope: Frontend
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #103720 +/- ##
========================================
Coverage 80.62% 80.62%
========================================
Files 9281 9281
Lines 396217 396190 -27
Branches 25252 25246 -6
========================================
- Hits 319442 319424 -18
+ Misses 76315 76306 -9
Partials 460 460 |
since both navigate and localStorage are mocked, and the internal state of the component was removed, we no longer see a real update reflected in the test; this works fine in the product
if you mock `onChange`, the label doesn't magically update anymore
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
static/app/views/performance/transactionSummary/transactionOverview/index.tsx
Outdated
Show resolved
Hide resolved
| const listProps = useMemo(() => { | ||
| if (multiple) { | ||
| return { | ||
| multiple, | ||
| value, | ||
| closeOnSelect, | ||
| onChange, | ||
| }; | ||
| } | ||
| return { | ||
| multiple, | ||
| value, | ||
| closeOnSelect, | ||
| onChange, | ||
| }; | ||
| }, [multiple, value, onChange, closeOnSelect]); |
There was a problem hiding this comment.
note: we can just keep everything in props and forward it as-is to List
| const wrapperRef = useRef<HTMLDivElement>(null); | ||
| // Set up list states (in composite selects, each region has its own state, that way | ||
| // selection values are contained within each region). | ||
| const [listStates, setListStates] = useState<Array<ListState<any>>>([]); |
There was a problem hiding this comment.
this was only used for clear, which now works by triggering onChange with an empty value (undefined for single select and [] for multi select)
| // react-aria turns all keys into strings | ||
| selectedKeys: defined(value) ? [getEscapedKey(value)] : undefined, | ||
| disallowEmptySelection: disallowEmptySelection ?? true, | ||
| selectedKeys: defined(value) ? [getEscapedKey(value)] : [], |
There was a problem hiding this comment.
note: this was a crucial fix - using [] for selctedKeys instead of undefined. Somehow, react-aria is not “reactive” when it sees undefined as value, I think it switches to an uncontrolled state.
since we want to be fully controlled, we never pass undefined to it.
It might make sense to switch to null for that reason, I haven’t tested if that would make it better, but since selectedKeys wants an array in any case, an empty array will always do what we want.
There was a problem hiding this comment.
Makes sense—would you mind adding this as an inline comment so we don't accidentally regress? Seems like an easy mistake to make.
| clearable | ||
| disallowEmptySelection={false} | ||
| menuTitle={t('Filter by category')} | ||
| onClear={() => setSelectedCategory(undefined)} |
There was a problem hiding this comment.
onChange does this automatically now
natemoo-re
left a comment
natemoo-re
left a comment
There was a problem hiding this comment.
Amazing cleanup, nice work! Just a few small suggestions for clarity but won't block.
| // react-aria turns all keys into strings | ||
| selectedKeys: defined(value) ? [getEscapedKey(value)] : undefined, | ||
| disallowEmptySelection: disallowEmptySelection ?? true, | ||
| selectedKeys: defined(value) ? [getEscapedKey(value)] : [], |
There was a problem hiding this comment.
Makes sense—would you mind adding this as an inline comment so we don't accidentally regress? Seems like an easy mistake to make.
There was a problem hiding this comment.
Bug: Optional template selector cannot be cleared
The disallowEmptySelection={false} prop was removed, but clearable was not added to this optional selector. This causes disallowEmptySelection to default to true, effectively preventing users from deselecting the template by clicking it, with no clear button available as a fallback.
static/app/views/detectors/components/forms/metric/templateSection.tsx#L119-L123
sentry/static/app/views/detectors/components/forms/metric/templateSection.tsx
Lines 119 to 123 in d796af9
Bug: Optional template selector cannot be cleared
The disallowEmptySelection={false} prop was removed, but clearable was not added to this optional selector. Since disallowEmptySelection now defaults to !clearable (true), users can no longer deselect the template by clicking it, and no clear button is rendered as a fallback.
static/app/views/detectors/components/forms/metric/templateSection.tsx#L119-L123
sentry/static/app/views/detectors/components/forms/metric/templateSection.tsx
Lines 119 to 123 in d796af9
@scttcper I think the bot is right, but I double checked and I’m not sure why sentry/static/app/views/detectors/components/forms/metric/templateSection.tsx Lines 136 to 139 in 0e46784 Since the types are now better, this can actually be removed because without I’ve compared the behavior on master and on this PR and the select behaves the same, which means: you can’t clear the value. I guess this makes sense because you need to Choose Your Metric. I’m just wondering why |
There was a problem hiding this comment.
Bug: Removal of empty selection support in template selector
disallowEmptySelection={false} was removed, but clearable was not added. This causes the component to default to disallowEmptySelection=true, making it impossible for users to clear the template selection despite the field being labeled as "optional".
static/app/views/detectors/components/forms/metric/templateSection.tsx#L121-L122
sentry/static/app/views/detectors/components/forms/metric/templateSection.tsx
Lines 121 to 122 in cfcb1ab
This PR does a couple of things (it got away from me a bit), but they all belong together:
onClearcallback ofcompactSelectinstead, we are “just” firing an
onChangeevent withundefinedfor single selection or[]for multi selection.compactSelectwere adjusted accordingly (+ type tests): if you passmultiple: true, you’ll always get an array intoonChange. But for single selects, we have another distributed union now so that you getSelectOption | undefinedifclearableistrueandSelectOptionifclearableisfalse.disallowEmptySelectionpropthis is a feature react-aria uses internally on collections; if set to false, it means you can click the current selection you have for single selections to get back to “no value”. This doesn’t make any sense if
clearableisfalse, because it’s just another way to clear values.I have now coupled this value to the
clearableprop to get consistent behavior: Ifclearableis set, you can also click the current selection to “unselect” it. If you’re not clearable, you can’t do this. Formultiple, nothing changes.nullandundefinedwhen a value gets clearedit’s now consistently
undefined, because we also havevalue={undefined}and it’s easier to instantiateuseStatewithundefined. Note that we used to sendnullwhen clearing. If we wantnullhere, I would also disallowundefinedforvalueand dovalue={null}instead. I don’t really care and typescript should guard us in either way, unless we have a lot ofany.Technical Note:
Omitbecame a problem becauseOmiton discriminated unions flattens the union and ruins the distribution. I’ve had to useDistributiveOmita bunch of times. I think it mostly shows that building types based on “I want all the types of this underlying thing except those 5” is really messy. The type hierarchy is very hard to understand. We sometimes even omitted things that weren’t any longer part of the union because they got removed. TypeScript doesn’t report on this. I also had to convert a couple ofinterfacestotypesto be able to useDistributiveOmit.Also: I had to update some tests that weren’t really managing any state and relied on the state being handled internally. This is not what happens in the product, because each instance manages state (as it should)