material-ui
material-ui copied to clipboard
mui
[FocusTrap] update getTabbable function return type
Part of: #42231
FocusTrap component: Change the return type of the getTabbable function from ReadonlyArray<string> to ReadonlyArray<HTMLElement>.
Netlify deploy preview
https://deploy-preview-42237--material-ui.netlify.app/
Bundle size report
No bundle size changes (Toolpad) No bundle size changes
Generated by :no_entry_sign: dangerJS against 004f1c1f752640cb88531f20456aff5ebbf8cd21
Hey @KalmarLorand thanks for taking a stab at this issue. When changing the types you need to run:
pnpm prototypes
pnpm docs:api
to ensure that the propTypes and the docs API are up to date. This will fix the CI failing checks.
Hey @KalmarLorand thanks for taking a stab at this issue. When changing the types you need to run:
pnpm prototypes pnpm docs:apito ensure that the propTypes and the docs API are up to date. This will fix the CI failing checks.
Hey, thanks for the heads-up. I've ran the commands but for some reason nothing no files were changed
No declaration for FocusTrapSlots Built API docs for /base-ui/react-focus-trap/components-api/#focus-trap
As far as I understand things, this PR should be:
- moved to https://github.com/mui/base-ui
FocusTrapreleased from@base_ui/reactas unstable@base_ui/react/unstable_FocusTrapused in Material UI v6FocusTrap's source removed from@mui/base- Documented in https://master--base-ui.netlify.app/base-ui/getting-started/ as either unstable or legacy
Or maybe alternatively, if clearer:
- moved to https://github.com/mui/base-ui
FocusTrapreleased from@mui/basefrom https://github.com/mui/base-uiFocusTrapsource removed from https://github.com/mui/material-ui- Documented in https://master--base-ui.netlify.app/base-ui/getting-started/ as either unstable or legacy
cc @michaldudak to get a sense of what's the strategy with merging @mui/base into @base_ui/react.
The safest option, IMO, would be to move the code of legacy Base UI components used in Material UI to the Material UI sources. We don't want to use any of the new Base UI components in Material UI just yet, as this could limit our options for API design and functionality. cc @colmtuite
The safest option, IMO, would be to move the code of legacy Base UI components used in Material UI to the Material UI sources.
@michaldudak What risks do you have in mind?
In the two options described above, it would be handled with two separate npm packages, "Component" for the new ones, "Component (legacy)" for the old ones:
The cons I see with having this code of a Focus Trap split between Material UI and Base UI:
- The person responsible for Focus Trap has to work more on two different repositories to maintain its product scope: https://www.notion.so/mui-org/Focus-Trap-fbe1aed295f748c19576d024e9a48c62 (maybe to change the owner). The biggest challenge might be that the GitHub issues the person needs to check are spread over two different repositories. Or maybe it will be that it's not possible to update both components with the same PR.
- For all the
@mui/baseusers, the migration path from this deprecated package might feel more obscure, or at least it doesn't feel like we play the strategy of carrying the community with us, by reducing the friction. Today, until Base UI gets parity with React Aria, this feels like our biggest differentiator, so it feels like we should play the card. The counterargument could be that maybe playing the Radix Primitives migration card is more effective, but if we look at the downloads: Radix Primitives (@radix-ui/react-slots - @storybook/components) vs. Base UI (@mui/base - @mui/material) it's not so clear which horse is the best to bet on (would migrate the most people). - We don't educate much developers about the existence of the Base UI brand, people using @mui/base would feel more like a Material UI thing. The counterargument could be that maybe we would carry that feeling we want to avoid to Base UI, but if it's done well, I don't think it has to feel like this.
An important constraint I think we should enforce is to have a single owner for the Focus Trap logic, meaning to have it handled similarly to how the maintainer of https://github.com/focus-trap/focus-trap masters this product scope. It shouldn't be an option to have two different owners of two different implementations of the same product scope.
Implementing https://github.com/mui/material-ui/pull/42237#issuecomment-2166760490 doesn't go against this constraint, but doesn't make it clear, so I highlighted it.
It may turn out we don't need a separate FocusTrap component in Base UI, so the current implementation used by Material UI can be moved to its sources. After we implement Material UI with Base UI, it could be removed altogether, as the logic will be baked in components such as the Dialog.
In the two options described above, it would be handled with two separate npm packages, "Component" for the new ones, "Component (legacy)" for the old ones
I'm not sure I'd put the legacy components docs in the same website. A subdomain with docs for @mui/base for existing users (similar to Material UI v4 docs) could work better. Or, (since AFAIK you agreed to put the new docs in a subdomain), we can leave the old docs in place (for @mui/base users) but clearly indicate they relate to the old version and link to the new docs (similarly to how React handles it: https://legacy.reactjs.org/docs/getting-started.html).
The current state of things isn't perfect, but we'll be in a much better place when we implement Material UI with @base_ui/react and have a single source of truth for the components logic. Only then I'd introduce cross-product component owners.
Today, until Base UI gets parity with React Aria, this feels like our biggest differentiator, so it feels like we should play the card. The counterargument could be that maybe playing the Radix Primitives migration card is more effective
Sorry but I don't understand this bullet point.
We don't educate much developers about the existence of the Base UI brand
I don't want to speak on behalf of @colmtuite, but I don't think we want to just yet. When we have the foundational components ready, we'll advertise Base UI much more aggressively.
I'm not sure I'd put the legacy components docs in the same website.
We definitely do not want the legacy components in the same website. We want a fresh, clean site, positioning it as a new product. We don't want anything old weighing it down on day 1.
Ideally, the experience shouldn't change at all for legacy base ui users. Same package, same domain name, same docs, same repo etc. All that changes is they get a note in the docs that new Base UI is live, and they should check it out.
The current state of things isn't perfect, but we'll be in a much better place when we implement Material UI with @base_ui/react and have a single source of truth for the components logic.
Agreed. The pain of duplication is only temporary. When all other libs are built on Base UI, things will be much simpler. But we should live with the pain of duplication for now, otherwise we will compromise Base UI.
I don't want to speak on behalf of @colmtuite, but I don't think we want to just yet. When we have the foundational components ready, we'll advertise Base UI much more aggressively.
We're not focused on or interested in education/marketing right now, because we have nothing to talk about. When we're close to launch, and have our own domain, then we'll begin marketing/educating.
An important constraint I think we should enforce is to have a single owner for the Focus Trap logic
It is critically important that the Base UI team owns all of Base UI. The way to enforce consistency, is to just have other libs be built on top of Base UI.
