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

Fixes #1190 - Use Sass variable and Sass mixin to generate is-variable rules #1480

Closed
wants to merge 2 commits into from

Conversation

ADTC
Copy link

@ADTC ADTC commented Nov 27, 2017

This is a improvement | bugfix | documentation fix. (all)

  • Improvement because, it drops the dependency on browser support for CSS variables. Depends purely on Sass pre-processing, so it will be compatible with browsers that don't have CSS variable support.
  • Bugfix because of the referenced issue.
  • Documentation fix because we can remove the warning about browser support in the is-variable docs.

Proposed solution

Instead of depending on CSS variable browser support, use a Sass variable and a Sass mixin to generate regular CSS for the is-variable class. This should increase browser compatibility as well as fix #1190, #1355 and #1181 where warnings are emitted when using a CSS variable. It will also make the deprecation warning in #1283 and #1210 completely irrelevant since that only applies to CSS variables.

Tradeoffs

I guess you have to learn how Sass variables and Sass mixins work? But other than that, I can't think of anything.

Testing Done

Injected the generated CSS code into Bulma Docs. Confirmed working:

bulma-is-variable-removed
bulma-is-variable-new-css

ADTC added 2 commits November 27, 2017 09:41
Instead of depending on CSS variable browser support, use a Sass variable and a Sass mixin to generate regular CSS for the is-variable class. This should increase browser compatibility as well as fix jgthms#1190 where warnings are emitted when using a CSS variable.
Since we moved from using CSS variable to Sass variable and Sass mixin, we no longer have to worry about browser support for CSS variables. The generated CSS is variable-free and will be compatible with more browsers.
@jgthms
Copy link
Owner

jgthms commented Nov 27, 2017

I know I could have done it this way but I want to use CSS variables because that’s the future. It’s also more elegant, and requires way fewer lines of code.

The solution here is to simply fix those deprecation warnings.

@jgthms jgthms closed this Nov 27, 2017
@ADTC
Copy link
Author

ADTC commented Nov 27, 2017

I don't know about the future, but what do you mean by "way fewer lines of code"? Looking at the comparison, the original code has 10 lines, vs the modified code has 11 lines (plus a blank line, but I won't count that).

That's merely a single line of code additional for several advantages. I don't see how saving 1 line gives you "way fewer lines". I don't see how it's more elegant either (one could say they are equally elegant in their own ways).

Also, if the effect can be achieved with simple CSS that is easily generated by the preprocessor, what advantage does using CSS variables serve? This is the only place in the entire project where a CSS variable is used, and IMHO its use is not even justifiable as necessary. It's simply a case of "I will because I can." 😞

@jgthms
Copy link
Owner

jgthms commented Nov 27, 2017

Surely you understand that I’m talking about the CSS output, not the source file…

Also, CSS variables have very good browser support, and it’s getting better. The only issue is the Sass compilation. Why not simply fix that?

Columns like this can also be implemented with floats. Why not use that then?

Bulma started with the idea of moving forward, with flexbox notably. This PR goes against that idea.

Repository owner locked and limited conversation to collaborators Nov 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with columnGap property
2 participants