-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ComboBox - @deephaven/components #2067
Conversation
3664f9d
to
ea02fa9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2067 +/- ##
==========================================
+ Coverage 46.45% 46.61% +0.16%
==========================================
Files 673 677 +4
Lines 38734 38556 -178
Branches 9815 9772 -43
==========================================
- Hits 17994 17974 -20
+ Misses 20688 20530 -158
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just needs to be labelled a breaking change and then migration support should be a little more detailed (particularly around placeholder
)
packages/components/src/ComboBox.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should be marked BREAKING, with migration instructions to transition to the new ComboBox
.
Pretty much just mapping the options
to children
and wrapping them in Item
? Would be good if you had an example.
I notice we don't have a spellCheck
prop anymore (seems to be false
by default, which is probably what we want anyway), and no more placeholder
props (Spectrum doesn't seem to like them) - what should we do when migrating those props?
import type { NormalizedItem } from '../utils'; | ||
import { PickerPropsT, usePickerProps } from '../picker'; | ||
|
||
export type ComboBoxProps = PickerPropsT<SpectrumComboBoxProps<NormalizedItem>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still want to template the props here so items get passed through correctly no?
Something like:
export type ComboBoxProps = PickerPropsT<SpectrumComboBoxProps<NormalizedItem>>; | |
export type ComboBoxProps<T extends NormalizedItem = NormalizedItem> = PickerPropsT<SpectrumComboBoxProps<T>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case? SpectrumComboBoxProps<NormalizedItem>
should already pass things through. We aren't extending NormalizedItem
so I think it should be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you're right - I was thinking if you pass in items that extended items and wanted the original item back, but that's not how this works anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See in these demos we have no placeholder
anymore. We should have instructions how to deal with that (default to the desired value instead? Display a validation error if blank with an appropriate warning?) and show those cases in the styleguide.
ComboBox
component in @deephaven/componentsPicker
sinceComboBox
is basically a subclassComboBox
component and replaced usage (condition column + row formatting. Also tested to make sure it still works as before)resolves #2065
BREAKING CHANGE: ComboBox component has been replaced.
To migrate to new version:
options
prop to define dropdown items. For cases where option value and display are the same, passing an array of values aschildren
will work. For cases where value and display differ,Item
elements must be passed as children. e.g.<Item key={value}>{display}</Item>
e.g.
spellcheck=false
prop is no longer supported or neededsearchPlaceholder
andinputPlaceholder
props are no longer supported and should be omitted. There is an optionaldescription
prop for cases where a descriptive label is desired. There is also alabel
prop for the primary component label.