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

[SASS] Change output style comments to non-rendered comments in invisibles files #2807

Merged
merged 4 commits into from
Jan 30, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jan 30, 2020

Summary

There are two kinds of SASS comments.

  1. // This style doesn't get output in the compiled CSS file
  2. /* This style does */

When importing the invisibles files (variables, mixins, functions), if those files contain the second comment style, then the comments will be output in the consumer's CSS. This is unnecessary and can cause confusion.

Kibana example:

Screen Shot 2020-01-30 at 15 05 10 PM

This PR, just changes the comments from the invisibles files to be the non-compiled style.

Checklist

  • [ ] Check against all themes for compatibility in both light and dark modes
  • [ ] Checked in mobile
  • [ ] Checked in IE11 and Firefox
  • [ ] Props have proper autodocs
  • [ ] Added documentation examples
  • [ ] Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cchaos cchaos added the chore label Jan 30, 2020
@cchaos cchaos requested a review from andreadelrio January 30, 2020 20:34
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Maybe we can have Sass Lint give us warnings when we use comments type 2 in our Sass files

@cchaos
Copy link
Contributor Author

cchaos commented Jan 30, 2020

Maybe we can have Sass Lint give us warnings when we use comments type 2 in our Sass files

That's a good thought, but it's hard to determine in the Linter if the file is simply just an invisibles file. For instance, we still use the compilable style of comments in selectors so when compiled we can view those comments like:

/**
* 1. Accounts for the border
*/
.euiBadge {
font-size: $euiFontSizeXS;
font-weight: $euiFontWeightMedium;
line-height: $euiSize + 2px; /* 1 */

But those will only be compiled once, where as the invisibles files are imported multiple times by multiple files and so we don't want those comments output multiple times without context.

@cchaos cchaos merged commit 08ca520 into elastic:master Jan 30, 2020
@cchaos cchaos deleted the invisibles_comments branch January 30, 2020 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants