Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Tabs] Overhaul Tabs API #245

Merged
merged 93 commits into from
Apr 29, 2024
Merged

[Tabs] Overhaul Tabs API #245

merged 93 commits into from
Apr 29, 2024

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Mar 25, 2024 •
edited
Loading

  • Replaced the slots API with the render prop.
  • Changed the selectionFollowsFocus prop to activateOnFocus and moved it to TabsList.
  • Added the loop prop to the Tabs.
  • Added the keepMounted prop to the TabPanel
  • Changed the directory structure to keep all components and hooks under top-level Tabs directory

To do:

  • Style hooks on subcomponents
  • Lava-lamp animation support
  • Rendering the indicator on the server
  • Hooks cleanup
  • Docs update
  • "uncontrolled component becoming controlled" problem when neither value nor defaultValue is set
  • keepMounted prop on TabPanels
  • Sliding tab panels -> in a separate PR
  • Scrollable tab list -> in a separate PR

Preview:

Closes #212
Closes #81

@michaldudak michaldudak added the component: tabs This is the name of the generic UI component, not the React module! label Mar 25, 2024
@michaldudak michaldudak mentioned this pull request Mar 27, 2024
Merged
1 task
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 27, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 27, 2024
@michaldudak michaldudak changed the title [Tabs] Replace slots with render props [Tabs] Overhaul Tabs API Mar 29, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 4, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 5, 2024
@michaldudak michaldudak marked this pull request as ready for review April 5, 2024 11:33
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 9, 2024
@michaldudak
Copy link
Member Author

I used data-activation-direction in the demos and docs and used another character for the arrow.
@colmtuite would you like to take another look before I merge this?
@atomiks, are you OK with the implementation?

@colmtuite
Copy link
Contributor

right when the active tab is to the right of the previously active tab (only applied when orientation=vertical).

I think this is a typo? Only applied when horizontal?

I'm still seeing a missing symbol
Screenshot 2024-04-25 at 23 26 39

@michaldudak
Copy link
Member Author

I think this is a typo?

Yup, just corrected it.

I'm still seeing a missing symbol

OK then, I'll play it safe

image

atomiks
atomiks reviewed Apr 26, 2024
View reviewed changes
packages/mui-base/src/Tabs/TabIndicator/useTabIndicator.ts Outdated
Comment on lines 25 to 26
const [, forceUpdate] = React.useState({});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an existing useForceRender util

atomiks
atomiks reviewed Apr 26, 2024
View reviewed changes
packages/mui-base/src/Tabs/TabIndicator/TabIndicator.tsx Outdated
Comment on lines 64 to 67
const prehydrationScript = `
(function() {
let indicator = document.currentScript.previousElementSibling;
if (!indicator) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be better to store the source of this in its own file (unused) and use a minified version here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a utility to minify and inline scripts in 30412c2

atomiks
atomiks reviewed Apr 26, 2024
View reviewed changes
packages/mui-base/src/Tabs/TabIndicator/TabIndicator.tsx
...other
} = props;
const render = renderProp ?? defaultRenderFunctions.span;
const [instanceId] = React.useState(() => Math.random().toString(36).slice(2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useId util hook?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't work during SSR in React 17 :(

atomiks
atomiks reviewed Apr 26, 2024
View reviewed changes
packages/mui-base/src/Tabs/Tab/Tab.types.ts Outdated
Comment on lines 17 to 19
* Callback invoked when new value is being set.
*/
onChange?: (event: React.SyntheticEvent, value: any) => void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, it should be better to prefer different prop names like onValueChange that passes in the value first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3a08692

@colmtuite
Copy link
Contributor

Ready to 🚀 on my side

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 26, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 29, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 29, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 29, 2024
@michaldudak michaldudak merged commit 5db5d3f into mui:master Apr 29, 2024
16 checks passed
@michaldudak michaldudak deleted the api-redesign/tabs branch April 29, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atomiks atomiks atomiks left review comments

@colmtuite colmtuite colmtuite approved these changes

Assignees
No one assigned
Labels
breaking change component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Tabs] Implement new API [Tabs] Widen value type to any
4 participants
@michaldudak @atomiks @colmtuite @oliviertassinari