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

Convert pagination component to CSS variables #35399

Merged
merged 3 commits into from
Feb 16, 2022
Merged

Convert pagination component to CSS variables #35399

merged 3 commits into from
Feb 16, 2022

Conversation

mdo
Copy link
Member

@mdo mdo commented Nov 25, 2021

This PR represents a potential shift in how we're using CSS variables for our components. In previous cases like #34600, I've been creating new CSS variables for each state-property pairing (e.g., --bs-btn-color, --bs-btn-hover-color, and --bs-btn-active-color). This matches our Sass-based customization of variables for almost every property and state, but it does create bloat in the component base class.

In contrast, this PR only sets the essential variables on the base class and uses CSS variable reassignment as needed to modify those properties for each state with their Sass variables. For example, --bs-pagination-color is set on .pagination and then overridden on .page-link:hover using the same --bs-pagination-color variable with a new value.

I think this approach makes more sense and can steer us toward more CSS variable re-use (e.g., --bs-pagination-bg: var(--bs-body-bg)), steering us away from too much Sass variable re-use that led to complicated customization issues.

Updated to use the same method we're using elsewhere, all in on the base class.

/cc @twbs/css-review


  • Revert to all CSS vars on base class
  • Update documentation

@mdo mdo requested a review from a team as a code owner November 25, 2021 23:43
@mdo mdo mentioned this pull request Nov 25, 2021
41 tasks
@Kopyov
Copy link
Contributor

Kopyov commented Nov 26, 2021

Although this solution looks cleaner, it makes customization and specifically overrides a bit more complicated as they are spread across the states. But considering the main advantage of generating less CSS variables, it's definitely a good step forward. Will other components get the same update anytime soon?

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Looks great as-is. I'm almost sure there's room for improvements, either on CSS or Sass side, but can't find them for now :)

scss/mixins/_pagination.scss Outdated Show resolved Hide resolved
scss/_pagination.scss Outdated Show resolved Hide resolved
@mdo mdo force-pushed the css-vars-pagination branch from 3caa923 to ce2936c Compare February 16, 2022 17:21
scss/_pagination.scss Outdated Show resolved Hide resolved
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Nice job 👌

@mdo mdo merged commit 48a7160 into main Feb 16, 2022
@mdo mdo deleted the css-vars-pagination branch February 16, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants