react-combobox: remove ComboButton component#23492
react-combobox: remove ComboButton component#23492smhigley merged 2 commits intomicrosoft:masterfrom
Conversation
📊 Bundle size report
🤖 This report was generated against 0d4794d3924d648c788993e087a48dd45fe10a1b |
|
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 2aca10a:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 0d4794d3924d648c788993e087a48dd45fe10a1b (build) |
| const useStyles = makeStyles({ | ||
| root: {}, | ||
| root: { | ||
| ...shorthands.border('1px', 'solid', 'transparent'), |
There was a problem hiding this comment.
Should 1px be replaced with one of the strokeWidth* tokens?
There was a problem hiding this comment.
probably :D. Do you mind if I make a separate PR for the style changes? This one makes it look like that's a change, but it's just a copy/paste of the ComboButton styles
| */ | ||
| const useStyles = makeStyles({ | ||
| root: {}, | ||
| root: { |
There was a problem hiding this comment.
The root slot is a div which has a default display value of block but all our input controls have a default display value of inline (or inline-block, inline-flex, etc).
Should this slot have display: inline-block set to make it consistent with other input controls?
| @@ -203,14 +211,14 @@ export const useCombobox_unstable = (props: ComboboxProps, ref: React.Ref<HTMLBu | |||
| onTriggerBlur?.(event); | |||
There was a problem hiding this comment.
These should be also renamed, right? (I mean onTriggerBlur, onTriggerClick and onTriggerKeyDown)
nit: but we can also do:
state.button.onBlur = useMergedEventCallbacks(
state.button.onBlur,
React.useCallback((event: React.FocusEvent<HTMLButtonElement>) => {
if (!ignoreTriggerBlur.current) {
setOpen(event, false);
}
ignoreTriggerBlur.current = false;
}, [])
);
In moving to a slots-based approach for Combobox/Dropdown, we talked about removing the ComboButton/ComboInput components in favor of directly putting the button/input and icon slots on the main Combobox/Dropdown components themselves. This has a few benefits:
You can see where this change fits into the full combo/dropdown update in this PR: #22906