-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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] Refactor the compound components building blocks #36400
Conversation
Netlify deploy previewhttps://deploy-preview-36400--material-ui.netlify.app/ @material-ui/unstyled: parsed: +1.45% , gzip: +0.39% Bundle size report |
@@ -235,5 +235,6 @@ export default function useButton(parameters: UseButtonParameters): UseButtonRet | |||
setFocusVisible, | |||
disabled, | |||
active, | |||
ref: handleRef, |
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 realized that it was impossible to use two hooks (useListItem
and useButton
) on the same element, as the refs returned by the hooks' getRootProps
would overwrite one another. Having a ref returned separately allows us to combine them.
@@ -244,19 +245,51 @@ function handleKeyDown<TOption>( | |||
}; | |||
|
|||
case 'ArrowUp': | |||
// TODO: extend current selection with Shift modifier | |||
if (orientation !== 'vertical') { |
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 may also add vertical-reverse
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 would to see a use-case for it :)
8cb3eab
to
f75b78e
Compare
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.
Thanks for the examples, they are very useful to reviewing the functionality. The new hooks make sense. I am not sure about the focusManagement
being in the useListBox
, I feel like it should be part of the hook/component using the useListBox
but I understand there will be lots of repetitions this way, so let's go with it :)
@@ -17,11 +17,11 @@ export interface TabsUnstyledOwnProps { | |||
* The value of the currently selected `Tab`. | |||
* If you don't want any selected `Tab`, you can set this prop to `false`. |
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.
* If you don't want any selected `Tab`, you can set this prop to `false`. | |
* If you don't want any selected `Tab`, you can set this prop to `null`. |
Is this correct with the current changes?
@@ -244,19 +245,51 @@ function handleKeyDown<TOption>( | |||
}; | |||
|
|||
case 'ArrowUp': | |||
// TODO: extend current selection with Shift modifier | |||
if (orientation !== 'vertical') { |
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 would to see a use-case for it :)
*/ | ||
tabsContextValue: TabsContextValue; | ||
contextValue: TabsProviderValue; |
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.
👍
3467ae8
to
f4df169
Compare
f3b0d21
to
b30051a
Compare
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.
Not related to this changes issues:
- Joy UI: https://deploy-preview-36400--material-ui.netlify.app/joy-ui/react-menu/ the menus in should focus the trigger button when escape is pressed
- Joy UI, we should improve the focus indicators on the following tabs demos:
- Base UI: https://deploy-preview-36400--material-ui.netlify.app/base/react-menu/ when choosing menu item by clicking, we should not be seeing the focusable indicator on the trigger element
@@ -1,6 +1,61 @@ | |||
{ | |||
"parameters": {}, | |||
"returnValue": {}, | |||
"returnValue": { |
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.
Finally, the return value is here :)
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'm curious why parameters aren't picked up, though. Let's investigate in another PR.
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'm curious why parameters aren't picked up, though. Let's investigate in another PR.
Addressed in #37034
"description": "false<br>| number<br>| string" | ||
} | ||
}, | ||
"defaultValue": { "type": { "name": "union", "description": "number<br>| string" } }, |
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.
Replacing false
with null
makes sense 👍
return [...selectedValues, item]; | ||
} | ||
|
||
// Truncate the selection to the limit (discard items with lower indexes). |
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 see, but selectionMode: 'none' | 'one' | 'multiple'
makes much more sense when reading the code, instead of figuring out that infinite is a valid number option for the multiple use-case. We can also name the options: selectionMode: 'none' | 'single' | 'multiple'
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.
🚢 it
I've created a PR to address this: #36847 |
Breaking changes
Menu
MenuUnstyledContext
is replaced byMenuProvider
. The value to pass to the provider is returned by theuseMenu
hook.onClose
prop is replaced byonOpenChange
. It has theopen
parameter and is called when a menu is opened or closedSelect
SelectUnstyledContext
is replaced bySelectProvider
. The value to pass to the provider is returned by theuseSelect
hook.SelectUnstyled
's popup is permanently mounted.defaultOpen
prop was added to the SelectUnstyled. The open/close state can now be controlled or uncontrolled, as avalue
.Tabs
TabsContext
is replaced byTabsProvider
. The value to pass to the provider is returned by theuseTabs
hook.null
to Tabs'value
prop, instead offalse
. This is consistent with how Select works.value
prop is still technically not mandatory on TabUnstyled and TabPanel, but when omitted, the contents of the selected tab panel will not be rendered during SSR.This started as a rewrite of Unstyled Tabs and related components to use context instead of
cloneElement
but turned into a significant refactoring of all the compound components: tabs, select, and menu.I've identified two cross-cutting concerns that I extracted to their own hooks: navigation within the list (
useListbox
, likely needing a rename touseList
oruseListNavigation
, anduseListItem
) and determining the children of a compound component (useCompoundParent
anduseCompoundItem
).Notable improvements
Demos
To do
Closes #35084
Closes #34393
Closes #33663
Closes #36799
Closes #35795