Skip to content

[Tabs] Refactor to useRenderElement #1881

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

Merged
merged 18 commits into from
May 29, 2025

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert added core DO NOT USE. This label has been deprecated and replaced by `all components` and `internal`. component: tabs labels May 7, 2025
Copy link

pkg-pr-new bot commented May 7, 2025

Open in StackBlitz

npm i https://pkg.pr.new/@base-ui-components/react@1881

commit: fb51e1b

Copy link

netlify bot commented May 7, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit fb51e1b
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/683846239a07a6000863dd04
😎 Deploy Preview https://deploy-preview-1881--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@mj12albert mj12albert marked this pull request as ready for review May 7, 2025 10:34
@@ -94,12 +203,12 @@ export const TabsIndicator = React.forwardRef(function TabIndicator(

export namespace TabsIndicator {
export interface State extends TabsRoot.State {
selectedTabPosition: ActiveTabPosition | null;
selectedTabSize: ActiveTabSize | null;
selectedTabPosition: TabPosition | null;
Copy link
Member

Choose a reason for hiding this comment

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

The TabPosition and TabSize types should be publicly exported as they can be used in a render function

import { TabsListContext } from './TabsListContext';

const EMPTY_ARRAY: number[] = [];

function getInset(tab: HTMLElement, tabsList: HTMLElement) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: could you move these helper functions below the component? This way, while reading the code we'll go from high to low level details.

@@ -104,7 +202,7 @@ export const TabsList = React.forwardRef(function TabsList(
export namespace TabsList {
export type State = TabsRoot.State;

export interface Props extends BaseUIComponentProps<'div', TabsList.State> {
export interface Props extends BaseUIComponentProps<'div', TabsRoot.State> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export interface Props extends BaseUIComponentProps<'div', TabsRoot.State> {
export interface Props extends BaseUIComponentProps<'div', State> {

@mj12albert mj12albert force-pushed the tabs/use-render-element branch from 71d70d4 to 87c9d5a Compare May 7, 2025 12:53
@mj12albert mj12albert requested a review from michaldudak May 7, 2025 13:01
@mj12albert
Copy link
Member Author

Fixed the comments ~ @michaldudak

export type TabActivationDirection = 'left' | 'right' | 'up' | 'down' | 'none';
export type TabValue = any | null;

export namespace TabsRoot {
export type State = {
orientation: TabsOrientation;
orientation: Orientation;
Copy link
Member

Choose a reason for hiding this comment

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

IMO we could re-export Orientation in the TabsRoot namespace, so types are colocated from devs' point of view.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, should types like TabValue/TabActivationDirection be namespaced under the root too then? (or the most suitable part) @michaldudak

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'd do it as well. However, currently the docs generator doesn't handle such cases well, but I'm going to spend some time on it next week and solve this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated and used the @type annotation for value, the other changes didn't afffect the props table

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 9, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 13, 2025
@mj12albert mj12albert requested a review from michaldudak May 20, 2025 08:08
propGetter: getRootProps,
render: render ?? 'div',
className,
const renderElement = useRenderElement('div', componentProps, {
Copy link
Member

Choose a reason for hiding this comment

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

We usually call the returned var element. Also, in line 92, it should not be called but returned directly (same in other files).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

@@ -121,7 +212,7 @@ export namespace TabsRoot {
* The component orientation (layout flow direction).
* @default 'horizontal'
*/
orientation?: TabsOrientation;
orientation?: Orientation;
Copy link
Member

Choose a reason for hiding this comment

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

Due to a current limitation of API extractor, we have to manually override the type with a @type JSDoc tag:

@type Tabs.Root.Orientation

'aria-controls': tabPanelId,
'aria-selected': selected,
id,
onClick(event: React.MouseEvent<HTMLButtonElement>) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's wrap these in useEventCallback so they are stable between renders.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

@michaldudak michaldudak added internal and removed core DO NOT USE. This label has been deprecated and replaced by `all components` and `internal`. labels May 29, 2025
@mui-bot
Copy link

mui-bot commented May 29, 2025

Bundle size report

Total Size Change:${\tiny{\color{green}▼}}$-4.68KB(-0.32%) - Total Gzip Change:${\tiny{\color{green}▼}}$-944B(-0.19%)
Files: 41 total (0 added, 0 removed, 2 changed)

@base-ui-components/react/tabsparsed:${\tiny{\color{green}▼}}$-2.48KB(-9.34%) gzip:${\tiny{\color{green}▼}}$-514B(-5.62%)
@base-ui-components/reactparsed:${\tiny{\color{green}▼}}$-2.2KB(-0.72%) gzip:${\tiny{\color{green}▼}}$-430B(-0.46%)

Details of bundle changes

Generated by 🚫 dangerJS against fb51e1b

@mj12albert mj12albert requested a review from michaldudak May 29, 2025 10:44
@mj12albert mj12albert merged commit 82baf05 into mui:master May 29, 2025
22 checks passed
@mj12albert mj12albert deleted the tabs/use-render-element branch May 29, 2025 11:48
@oliviertassinari oliviertassinari added core DO NOT USE. This label has been deprecated and replaced by `all components` and `internal`. and removed internal labels May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tabs core DO NOT USE. This label has been deprecated and replaced by `all components` and `internal`.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants