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

Use escape-svg() function #29077

Merged
merged 3 commits into from
Jul 20, 2019
Merged

Use escape-svg() function #29077

merged 3 commits into from
Jul 20, 2019

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Jul 18, 2019

Use escape-svg() to escape <, > & # characters. This makes the _variables.scss more readable & maintainable. Also includes a tiny property optimalization for the select background (see https://www.diffchecker.com/7IBMI8dF).

Fixes #29065

@MartijnCuppens MartijnCuppens marked this pull request as ready for review July 18, 2019 18:38
@MartijnCuppens MartijnCuppens requested a review from a team as a code owner July 18, 2019 18:38
@voltaek
Copy link
Contributor

voltaek commented Jul 18, 2019

I think it would be cleaner if escape-svg() was used when defining the variable containing the SVG markup rather than upon each use of each of those variables throughout the code.

Doing so would also mean those using the variables containing the SVG markup elsewhere don't have to know to wrap the use of the variable in escape-svg(), saving them some headache.

@mdo
Copy link
Member

mdo commented Jul 18, 2019

Hmm, perhaps there's another function or mixin we could use to handle that? Or is this over-engineering? I like the idea of keeping the variables super clean and the source Sass.

@mixin svg-bg($variable) {
  background-image: escape-svg($variable);
}

.element {
  @include svg-bg($variable-name);
}

@voltaek
Copy link
Contributor

voltaek commented Jul 18, 2019

@mdo You'd need multiple mixins or an additional parameter to your suggested the mixin to accommodate the SVG variables being used with background, background-image, and content (currently in the BS source), so I think that would be over-engineering.

@MartijnCuppens
Copy link
Member Author

Using a mixin would make the code less readable imo and we lose the ability to use our property order linting. Edit: and we 'll need more mixins, like @voltaek mentioned.

I've moved the functions to our codebase instead of the _variables.scss file for the following reasons:

  • It just looks cleaner in the variables file
  • We automate the escaping, even if a developer forgets to do this.
  • We might make the _variables.scss file work independent from _functions.scss in the future, this would be a first step

@mdo
Copy link
Member

mdo commented Jul 19, 2019

Gotcha, makes sense! I’d like to push a couple changes to the docs page before merging to clearly explain and illustrate the function.

@mdo mdo merged commit f6694b7 into master Jul 20, 2019
@mdo mdo deleted the master-mc-svg-escape branch July 20, 2019 01:57
XhmikosR pushed a commit that referenced this pull request Jul 26, 2019
XhmikosR pushed a commit that referenced this pull request Jul 26, 2019
XhmikosR pushed a commit that referenced this pull request Jul 30, 2019
@mdo mdo mentioned this pull request Jul 30, 2019
XhmikosR pushed a commit that referenced this pull request Jul 30, 2019
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.

Make use of svg-escape function
4 participants