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

chore: drop normalize-scss and upgrade to normalize.css v8.0.0 #11086

Merged
merged 1 commit into from
Mar 25, 2018

Conversation

ncoden
Copy link
Contributor

@ncoden ncoden commented Mar 23, 2018

normalize-scss is not maintained and actually pollute the Sass namespace. Move back to a as-hard-to-maintain approach with a Sass mixin hosting the normalize.css code.

Note: /* */ comments where replaced by // comments to not being output.

Changes

  • Remove normalize-scss dependencies
  • Create vendor/normalize SCSS file hosting the normalize v8.0.0 CSS inside a mixin
  • Make only the root ./vendor folder being ignored by git

Closes #11015
Replaces #11016
Replaces #11072

`normalize-scss` is not maintained and actually pollute the Sass namespace. Move back to a as-hard-to-maintain approach with a Sass mixin hosting the normalize.css code.

Note: `/* */` comments where replaced by `//` comments to not being output.
Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

Looks good so far but we may have to check all exaples for layout changes due to the new normalize version.

@ncoden
Copy link
Contributor Author

ncoden commented Mar 24, 2018

@DanielRuf
Copy link
Contributor

DanielRuf commented Mar 24, 2018

Sidenote: https://github.com/zurb/foundation-sites/blob/ab4beb10bb91034c76dcf0457c105766d8ef8b47/scss/components/_reveal.scss#L42-L50 is output before the other styles. Should we move this to another place or is it still needed (there)?

See the generated foundation.css file at the very top.

@ncoden
Copy link
Contributor Author

ncoden commented Mar 24, 2018

@DanielRuf This is related to #10856 and components sorting. It's at the top because placeholders code is generated (if used) where it is placed. The problem in this case is that it is used inside a modifier mixin (reveal-modal-width), not the main one (foundation-reveal) and I guess the modifier mixin should be usable without the main one...

So, it seems it's part of an "initialization" step (after import and before each component generation) that does not exist in our architecture (and most SCSS architectures). That's too deep for v6 but a good challenge for v7 :). It's does not cause any bugs, right ?

@DanielRuf
Copy link
Contributor

It's does not cause any bugs, right ?

It adds the styles even if none of the mixins is used so it may cause issues.

@ncoden
Copy link
Contributor Author

ncoden commented Mar 24, 2018

hmm... No it shouldn't. That's a placeholder, it generate styles only if used. And empty breakpoints are removed by Sass. Did you test it ?

@DanielRuf
Copy link
Contributor

components sorting. It's at the top because placeholders code is generated (if used) where it is placed

This is not the case. The import statement alone adds this directly.

See #11087

@ncoden
Copy link
Contributor Author

ncoden commented Mar 24, 2018

This is not the case. The import statement alone adds this directly.

Did you test it ?

@DanielRuf
Copy link
Contributor

At least it makes not much sense to add it up there. The other code in my PR works too. Deduplication is done by libsass.

@DanielRuf
Copy link
Contributor

Anyway, we should check that the side effects of the normalize update on the components.

@DanielRuf
Copy link
Contributor

This is not the case. The import statement alone adds this directly.

Did you test it ?

Probably it was added because the docs and default foundation.scss use it with the include statement.

@ncoden ncoden merged commit 8b6a512 into foundation:develop Mar 25, 2018
@ncoden ncoden deleted the chore/normalize-8 branch April 7, 2018 17:41
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Jun 16, 2018
…6.5.0

6e72435 chore: drop `normalize-scss` and upgrade to normalize.css v8.0.0

Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
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.

2 participants