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

CSS: rename some variables to be consistent #37804

Merged
merged 2 commits into from Jan 6, 2023

Conversation

louismaximepiton
Copy link
Member

Description

Rename each -2xl to -xxl.

Motivation & Context

Be consistent with other variables.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

NA

@louismaximepiton louismaximepiton requested a review from a team as a code owner January 4, 2023 15:12
@louismaximepiton
Copy link
Member Author

Is there any way to deprecate those CSS variables ?

@julien-deramond
Copy link
Member

Haven't reviewed precisely the PR but I agree with the principle here for consistency through the whole framework.

@twbs/css-review I'm not too familiar with when we authorize to deprecated some Sass vars like this. I'd say that in this case, it would be OK to deprecate them in v5.3 and not wait until v6. But a second opinion would help me :)

Is there any way to deprecate those CSS variables ?

Right now, I believe that only our migration guide do the job

@mdo
Copy link
Member

mdo commented Jan 4, 2023

This is an interesting one because one request we had late in v5's development to consider for v6 is that we use 2xl instead of xxl as it might scale better to even larger sizes like 3xl, 4xl, etc. I hate the inconsistency, but maybe we could reassign the old CSS and Sass vars to the new value, and then back track in v6? Thoughts?

@julien-deramond
Copy link
Member

consider for v6 is that we use 2xl instead of xxl as it might scale better to even larger sizes like 3xl, 4xl, etc.

Oh yeah nice, it will be better like this indeed.

I hate the inconsistency, but maybe we could reassign the old CSS and Sass vars to the new value, and then back track in v6? Thoughts?

Works for me, sounds like a good strategy

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

I think these two comments address it?

scss/_root.scss Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
@mdo mdo merged commit 8265927 into twbs:main Jan 6, 2023
@louismaximepiton louismaximepiton deleted the main-lmp-rename-css-variables branch January 9, 2023 07:29
@mahilanmjd mahilanmjd mentioned this pull request Apr 16, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants