8000 feat(Tabs)!: Add support for additional customization settings by rfgamaral · Pull Request #903 · Doist/reactist · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(Tabs)!: Add support for additional customization settings #903

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
Merged
merged 13 commits into from
Apr 17, 2025
Merged

feat(Tabs)!: Add support for additional customization settings #903

merged 13 commits into from
Apr 17, 2025

Conversation

rfgamaral
Copy link
Member
@rfgamaral rfgamaral commented Apr 16, 2025 •
edited
Loading

Short description

This PR adds support for the following customization settings for the Tabs component:

  • CSS variables for hover and disabled states for the Tab component
    • --reactist-tab-themed-hover / --reactist-tab-neutral-hover
    • --reactist-tab-themed-disabled / --reactist-tab-themed-disabled
    • A tab can be individually disabled with the disabled prop
  • CSS variable for a box shadow for the selected tab
    • --reactist-tab-neutral-shadow / --reactist-tab-themed-shadow
  • Full width for the TabList component
    • The width prop can be full or maxContent (default)
  • Tab alignment for the TabList component
    • The align prop can be start (default), center, or end
  • CSS variable for a transition for the selected tab
    • --reactist-tab-selected-transition
  • exceptionallySetClassName prop added to the TabList component
    • Allows to override the --reactist-tab-border-radius variable used in the "track" element

All these changes are necessary to help customize the Tabs component to help with the upcoming style updates, and they come with sensible defaults that match the current style and behaviour. This avoids breaking changes, and helps with a smoother transition. I've also added a few example stories to demonstrate some of the most relevant new settings.

PR Checklist

  • Updated docs (storybooks, readme)
  • Reviewed and approved Chromatic visual regression tests in CI

@rfgamaral rfgamaral self-assigned this Apr 16, 2025
@rfgamaral rfgamaral requested review from a team and frankieyan and removed request for a team April 16, 2025 14:49
@rfgamaral rfgamaral marked this pull request as ready for review April 16, 2025 14:49
frankieyan
frankieyan approved these changes Apr 17, 2025
View reviewed changes
Copy link
Member
@frankieyan frankieyan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good 👍 comments are not blocking but I'd like to see the tab's styles not break when resized. Perhaps there is a possible CSS-only solution?

  • exceptionallySetClassName prop added to the TabList component
    • Allows to override the --reactist-tab-border-radius variable used in the "track" element

Is the border radius point meant to be a sub-bullet item here or are they separate points? I'm confused because we should be able to just override the --reactist-tab-border-radius without the class name override 😁

@rfgamaral
Copy link
Member Author

Is the border radius point meant to be a sub-bullet item here or are they separate points? I'm confused because we should be able to just override the --reactist-tab-border-radius without the class name override 😁

The --reactist-tab-border-radius variable is used in two places within the Tabs component, one of those is the .track class, and that .track class is used here. Without the exceptionallySetClassName you can't override that variable unless you create a wrapper container around the TabsList component (or the whole Tabs component), and you would be doing that just to override this variable, which doesn't make a lot of sense to me.

So, no, they are not separate points. The exceptionallySetClassName was added so that we are able to override --reactist-tab-border-radius in all places that this variable is used. If you have a suggestion for a different approach, I'm open to ideas :)

@frankieyan
Copy link
Member

Ah ok, I think I get it - if we want to be able to apply different border radius values to the track and to the selected tab, perhaps separating it into two variables could be a solution, i.e. --reactist-tab-track-border-radius and --reactist-tab-selected-border-radius. It'd make it more explicit that this is a customization we support than the exceptionallySetClassName path.

@rfgamaral
Copy link
Member Author

if we want to be able to apply different border radius values to the track and to the selected tab, perhaps separating it into two variables could be a solution

The new design uses the same radius for both, but I'll split them for extended customizability :)

It'd make it more explicit that this is a customization we support than the exceptionallySetClassName path.

It doesn't change the fact that we still need this, though.

@rfgamaral rfgamaral changed the title feat(Tabs): Add support for additional customization settings feat(Tabs)!: Add support for additional customization settings Apr 17, 2025
@rfgamaral rfgamaral merged commit 7f60d81 into main Apr 17, 2025
7 checks passed
@rfgamaral rfgamaral deleted the ricardo/improve-tabs-customization branch April 17, 2025 19:46
@github-actions github-actions bot mentioned this pull request Apr 17, 2025
Merged
@frankieyan
Copy link
Member

Okay sorry I am finally starting to get it 😅 This problem would exist for all of the other exposed custom properties and not just for the border radius variables though, correct?

I'm not against having the exceptionallySetClassName prop available but I want to fully understand the use case.

Is the border radius something we want to customize per instance, or app-wide? If it's the latter, we could always configure the new values based on the global class added in https://github.com/Doist/todoist-web/pull/13368, and if it's the former, and I'd be curious why we'd do this, then having the class name makes more sense.

frankieyan
frankieyan reviewed Apr 17, 2025
View reviewed changes
@rfgamaral
Copy link
Member Author
rfgamaral commented Apr 17, 2025 •
edited
Loading

This problem would exist for all of the other exposed custom properties and not just for the border radius variables though, correct?

Not necessarily.

All the other properties are used under elements that either expose a className or a exceptionallySetClassName prop. The TabList component didn't expose either, that's why I need it, because the track element inside that component uses a variable that I need to customize, and I want to make that customization contained in the wrapper component I was creating.

Is the border radius something we want to customize per instance, or app-wide?

It will be app-wide.

If it's the latter, we could always configure the new values based on the global class added in https://github.com/Doist/todoist-web/pull/13368

Hum, this is a good point. I was approaching this problem differently, but now that you mention this, I see how the same result could be achieved without the need for exceptionallySetClassName.

Since I've already published a new Reactist release, I'll test both approaches and see how they look. If I end up not using exceptionallySetClassName, I can remove it. Or we can just keep it around "just in case" since it's in the name that it should be used "exceptionally" 😅 What do you think?

@frankieyan
Copy link
Member

Or we can just keep it around "just in case" since it's in the name that it should be used "exceptionally" 😅 What do you think?

No harm in leaving it in place I think 👍

@rfgamaral rfgamaral mentioned this pull request Apr 17, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankieyan frankieyan frankieyan approved these changes

Assignees

@rfgamaral rfgamaral

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rfgamaral @frankieyan
0