Combobox vNext styles#22461
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2d1c290:
|
📊 Bundle size report
🤖 This report was generated against 65926a5270c19e25bbc0931253ca1c4a90b0fb01 |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 65926a5270c19e25bbc0931253ca1c4a90b0fb01 (build) |
khmakoto
left a comment
khmakoto
left a comment
There was a problem hiding this comment.
Approved with very minor comments 😄
| displayName: 'ComboButton', | ||
| primarySlot: 'content', | ||
| // don't test deprecated className export on new components | ||
| disabledTests: ['component-has-static-classname-exported'], |
There was a problem hiding this comment.
@TristanWatanabe We should bring up removing this test at the next v-build team meeting.
| ...shorthands.padding('4px'), | ||
| // bottom focus border, shared with Input, Select, and SpinButton | ||
| '::after': { | ||
| boxSizing: 'border-box', |
There was a problem hiding this comment.
Nit: With absolute positioning, do you need boxSizing set?
There was a problem hiding this comment.
it's because of the height + border, the absolute positioning doesn't affect the internal sizing.
| flexGrow: 1, | ||
| backgroundColor: 'transparent', | ||
| ...shorthands.border('none'), | ||
| alignItems: 'center', |
There was a problem hiding this comment.
Nit: Consider also setting alignContent. My understanding is there are cases where alignContent is meaningful when the items don't take up all the available space.
There was a problem hiding this comment.
alignContent affects multi-line flex alignment, which isn't allowed in the combobox trigger/faceplate. I added an explicit flexWrap: nowrap to the styles to make that clearer 👍
| const optionValue = getValueString(value, props.children); | ||
|
|
||
| // check icon | ||
| let CheckIcon = <CheckmarkFilled />; |
There was a problem hiding this comment.
Consider if React.memo would be good to use here.
There was a problem hiding this comment.
I don't think we memoize icons elsewhere right now, so I think I'll leave as-is for now. It'd be an interesting sync question though :D
There was a problem hiding this comment.
I know @tomi-msft is looking at icon rendering performance. He may have some guidance here.
6 participants
Part of the Combobox convergence epic (#18133), adds styles for the combobox button & listbox/options dropdown.