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.

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

[tabs] Modernize implementation #751

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Oct 22, 2024

Summary

@mj12albert mj12albert added the component: tabs This is the name of the generic UI component, not the React module! label Oct 22, 2024
@mui-bot
Copy link

mui-bot commented Oct 22, 2024

Netlify deploy preview

https://deploy-preview-751--base-ui.netlify.app/

Generated by 🚫 dangerJS against ab269fd

@mj12albert mj12albert force-pushed the refactor/tabs-composite branch 4 times, most recently from 4dbf848 to 16c8fbb Compare October 28, 2024 09:52
@mj12albert mj12albert force-pushed the refactor/tabs-composite branch 4 times, most recently from c31706a to 5a7f24f Compare November 13, 2024 16:35
@mj12albert

This comment was marked as outdated.

@@ -170,7 +172,9 @@ export function useCompositeRoot(params: UseCompositeRootParameters) {
}

if (nextIndex !== activeIndex && !isIndexOutOfBounds(elementsRef, nextIndex)) {
event.stopPropagation();
Copy link
Member Author

Choose a reason for hiding this comment

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

@atomiks Why is stopPropagation() needed here, and is it ever ok to force propagate it? This breaks a test

Given this structure:

<Tabs.Root onKeyDown={handleKeyDown}>
  <Tabs.List>
    <Tabs.Tab value={0} />
    <Tabs.Tab value={1} />
  </Tabs.List>
  {/* tab panels */}
</Tabs.Root>

Tabs.List is a CompositeRoot, so on arrow keys, the event doesn't propagate and handleKeyDown doesn't get called

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not certain on that. It may not be necessary in most cases

Copy link
Member Author

Choose a reason for hiding this comment

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

OK ~ let's let it propagate by default, but I've put a param around it just in case:

if (stopEventPropagation) {
  event.stopPropagation();
}

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 19, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 19, 2024
@mj12albert mj12albert changed the title [tabs] Replace useCompound with Composite [tabs] Modernize implementation Nov 19, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 19, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 19, 2024
@mj12albert mj12albert force-pushed the refactor/tabs-composite branch 3 times, most recently from a2a0bfe to f00fd3d Compare November 20, 2024 12:38
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 20, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 20, 2024

const ownerState: CompositeItem.OwnerState = React.useMemo(
() => ({
active: index === activeIndex,
highlighted: index === activeIndex,
Copy link
Member

Choose a reason for hiding this comment

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

There's now a mismatch in naming: "highlighted" and "activeIndex". It should be either "active" / "activeIndex" or "hightlighted" / "highlightedIndex". I like "active" as it's shorter, even though it doesn't match the HTML/CSS meaning of active (an in :active). Alternatively, we could call it "focused"

Copy link
Member Author

@mj12albert mj12albert Nov 22, 2024

Choose a reason for hiding this comment

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

This originally came up here to match the Menu components ~ I'll change the names to match highlighted

packages/react/src/Composite/Item/CompositeItem.tsx Outdated Show resolved Hide resolved
packages/react/src/Composite/Root/CompositeRoot.tsx Outdated Show resolved Hide resolved
Comment on lines 125 to 126
// TODO: recalculate this using Composite stuff, but is it really needed?
totalTabsCount: -1,
Copy link
Member

Choose a reason for hiding this comment

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

AFAIR the idea was to have a custom attribute on the first and last tabs to be able to style them differently, but this isn't really needed now. We can add this in the future if there's a community interest.

packages/react/src/utils/useOnMount.ts Outdated Show resolved Hide resolved
@michaldudak
Copy link
Member

You can remove the useList and useCompound hooks, as they are no longer used anywhere.

@mj12albert mj12albert force-pushed the refactor/tabs-composite branch 6 times, most recently from 26bba8c to 65b1053 Compare November 22, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants