-
Notifications
You must be signed in to change notification settings - Fork 64
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(component): add multiselect #200
Conversation
There are two breaking tests I need to fix. Feel free to review while I 👀 . |
|
||
return ( | ||
React.isValidElement(trigger) && | ||
React.cloneElement<React.HTMLAttributes<any> & React.RefAttributes<any>>(trigger as any, { | ||
'aria-haspopup': true, | ||
'aria-haspopup': 'listbox', |
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.
aria-haspopup
can only be true/false
per the spec:
https://www.w3.org/WAI/PF/aria/states_and_properties#aria-haspopup
Is there a reason why it's not? 🤔
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.
Most other libraries I saw used listbox
instead of true
. However, looks like VO works either way so I'll return it to spec.
box-sizing: border-box; | ||
color: ${({ theme }) => theme.colors.secondary70}; | ||
flex: 1; | ||
height: 26px; |
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.
Any reason for not using theme.spacing
for the height still?
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.
There's no way to calculate 26px
using the current values of theme.spacing
. Will use remCalc
.
); | ||
} | ||
} | ||
export const List: React.FC<ListProps> = memo( |
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.
Nice change to FC!
3ba3058
to
b163039
Compare
} | ||
|
||
private renderChips() { | ||
const { chips, onChipDelete } = this.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.
🍹 const { chips = [], onChipDelete } = this.props;
and get rid of the if
.
box-sizing: border-box; | ||
color: ${({ theme }) => theme.colors.secondary70}; | ||
flex: 1; | ||
height: ${remCalc(26)}; |
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.
Does xLarge
look pretty different? Worst case can we do a large + xxSmall
?
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 can do 24 or 28, but I guess it would deviate from designs. Do you think it's worth it?
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.
@valterfatia Thoughts on this?
498e04c
to
3e2150f
Compare
3e2150f
to
b5514d7
Compare
a29a488
to
6290f88
Compare
Multi selectable component. Using the same
select
component but with an addedmulti
prop that enablescheckboxes
on all items. Allows multiple checked elements.Still using the same
value
andonItemClick
props to return the selected items. In the case of being amulti
select we return anarray
of values selected.