-
Notifications
You must be signed in to change notification settings - Fork 1.4k
wip: allow custom react element for S2 Picker value #9541
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
base: main
Are you sure you want to change the base?
Conversation
| * Custom renderer for the selected value shown in the button. | ||
| * Matches the `SelectValue` children render type. | ||
| */ | ||
| renderValue?: SelectValueProps<T>['children'] |
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 should just pass in the selected items (i.e. (selectedItems: T[]) => ReactNode) here, and only call it when there is at least one item selected. We already have the placeholder prop to customize the placeholder text, and you can omit renderValue if you want the default text. Is that sufficient for your use case or did you need one of the other render props?
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.
Yes, it's enough for our use case -- was probably being too proactive trying to think of a general situation.
0645caf to
7f8c3fd
Compare
7f8c3fd to
d5c4f3c
Compare
BrknRules
left a comment
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.
Some notes on implementation choices -- would appreciate thoughts!
| display: 'flex', | ||
| alignItems: 'center' | ||
| alignItems: 'center', | ||
| height: '100%' |
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.
Locked height to picker button height. Enough for our use case as we don't have anything larger than the button that should cause it to resize.
| <SelectValue | ||
| className={ | ||
| valueStyles({isQuiet}) + | ||
| (renderValue ? '' : ' ' + raw('&> :not([slot=icon], [slot=avatar], [slot=label], [data-slot=label]) {display: none;}')) |
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 was quite restrictive in allowing us our own layouts. This should allow custom layouts only through renderValue and not via slot manipulation.
This adds a
renderValueprop to Picker S2, which allows users to render custom elements within the Picker's select based off of selected items (could also just render a custom element regardless of selection as well, TBD).To Do
renderSelectedItems,renderValue, ...?)renderValuetype, atm just usesSelectValue.childrenbut should restrict