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

Add Sass variable for CSS variable prefix #31684

Merged
merged 3 commits into from
Sep 30, 2020
Merged

Add Sass variable for CSS variable prefix #31684

merged 3 commits into from
Sep 30, 2020

Conversation

mdo
Copy link
Member

@mdo mdo commented Sep 17, 2020

Suggestion from @8nix at #26596 (comment).

Question though: should we include the trailing - in the prefix variable? Thinking we either do this, or detect if it's nil and remove the dash. Leaning towards the former and requiring that we include it.

Thoughts @twbs/css-review?

@ffoodd
Copy link
Member

ffoodd commented Sep 17, 2020

We may invert your suggestion too, adding the dash if a string is set—here's a SassMeister.

Not sure if it's better or not, just another possible way :)

@mdo mdo marked this pull request as ready for review September 17, 2020 16:07
@mdo mdo requested a review from a team as a code owner September 17, 2020 16:07
@MartijnCuppens
Copy link
Member

Question though: should we include the trailing - in the prefix variable? Thinking we either do this, or detect if it's nil and remove the dash. Leaning towards the former and requiring that we include it.

I'm ok with the current implementation. We'll need the prefix in other files like https://github.com/twbs/bootstrap/blob/main/scss/_tables.scss too, not sure if there are any other files (read: too lazy to look it up 😄)

@mdo
Copy link
Member Author

mdo commented Sep 17, 2020

Good point @MartijnCuppens, might undo the last change here and keep it with the - in there given the prefix is used throughout the project. Will keep thinking!

Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

LGTM

@XhmikosR
Copy link
Member

Just a reminder (mostly for myself since I usually go on a merging spree 😁) some PRs target a next release like alpha3.

@XhmikosR
Copy link
Member

@mdo this needs a rebase

@M055277702

This comment was marked as spam.

@XhmikosR XhmikosR merged commit 3cf51c6 into main Sep 30, 2020
@XhmikosR XhmikosR deleted the variable-prefix branch September 30, 2020 05:33
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
* Add Sass variable for CSS variable prefix

* Update other --bs-* var instances
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.

5 participants