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

Switch to Sass and Stylelint #2530

Merged
merged 5 commits into from
Oct 14, 2019
Merged

Switch to Sass and Stylelint #2530

merged 5 commits into from
Oct 14, 2019

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Sep 7, 2019

I open this now, but requires some of my other PRs merged.

I will split the output folder change to a separate PR. See #2544


Switch to Sass.

Also:

  • switch to double quotes
  • remove leading zeros
  • use double colon pseudo selectors
  • merge a few selectors
  • restored prism-tomorrow.css and moved it in vendor

DO NOT SQUASH

@XhmikosR
Copy link
Contributor Author

This is ready, but requires #2544 to be merged first.

I checked the (beautified) CSS and everything is fine. The diff can be seen here https://gist.github.com/XhmikosR/b22af72fabd48b1b5584bf9d736d7c05/revisions

@XhmikosR XhmikosR mentioned this pull request Sep 29, 2019
@XhmikosR XhmikosR marked this pull request as ready for review September 29, 2019 11:37
@XhmikosR
Copy link
Contributor Author

This can be merged after #2625 and after a rebase.

@XhmikosR XhmikosR mentioned this pull request Oct 1, 2019
4 tasks
@Trott
Copy link
Member

Trott commented Oct 9, 2019

@nodejs/website

@XhmikosR
Copy link
Contributor Author

Here's the latest diff of the generated unminified CSS https://gist.github.com/XhmikosR/10aaa5ea030adfc87838daad111335e4/revisions

@phillipj
Copy link
Member

Great job! 💯 Changes looks okey at a glance.

GitHub's diff view gives the impression git thinks the original .styl files are deleted, which is sad cause we'd in practise loose the history of the style changes done throughout the project's lifetime so far. Think it would be valuable if we could make git understand the files has been renamed, instead of delete (and new .scss files introduced), since that would make it simpler to follow the history back when needed sometime in the future.

I would think git mv would do the trick in the regard:

$ git mv layouts/css/_base.styl layouts/css/_base.scss

Any thoughts?

@phillipj phillipj closed this Oct 11, 2019
@phillipj phillipj reopened this Oct 11, 2019
@phillipj
Copy link
Member

..sorry, I obviously pressed the wrong button while posting the comment above.

@XhmikosR
Copy link
Contributor Author

I think this happens because the files contain a lot of changes, so they are basically new.

@XhmikosR
Copy link
Contributor Author

I actually tried this with git mv and it's the same. The files have many changes that git considers them as new.

@phillipj
Copy link
Member

Hm okey, thanks for testing! From a distance it didn't look like too much changes to the content, but guess it's on the borderline of what makes git consider it a new file.

The only trick I can think of what that will allow us to follow the history of back to the original files, is to first rename the files as is and commit, then make the actual content changes as a second commit.

I've generally made a habit of making sure git log --follow -- <whatever-filename> can be used to track historical changes later when applicable. Whether or not it's worth the extra effort in this scenario is up to you.

@Trott
Copy link
Member

Trott commented Oct 12, 2019

The only trick I can think of what that will allow us to follow the history of back to the original files, is to first rename the files as is and commit, then make the actual content changes as a second commit.

That would mean we'd have a broken build in the first commit, though, right? We don't do much git bisect in this repo though, so maybe that's OK?

@phillipj
Copy link
Member

phillipj commented Oct 12, 2019 via email

@XhmikosR
Copy link
Contributor Author

The build won't fail since the second patch in the batch will be the Sass switch.

I will do it later, just remember to not squash the patches, otherwise it will be moot :P

This is for the upcoming Sass switch.

Also restore prism-tomorrow.css
Also:

* switch to double quotes
* remove leading zeros
* use double colon pseudo selectors
* merge a few selectors
* restored prism-tomorrow.css and moved it in vendor
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 12, 2019

I fixed a couple more issues I noticed, namely the server watch for scss files and README.md and TRANSLATION.md references to stylus

@phillipj
Copy link
Member

Looks great! Although GitHub's UI suggests the old files are deleted and completely new ones are added, it works as expected locally;

$ git log --oneline --follow -- layouts/css/_base.scss
d642361e (HEAD -> pr/2530, upstream/pr/2530) Switch to Sass.
7a40f336 Rename *.styl files to *.scss
1d687cf6 Stylus: always use variables for colors. (#2528)
25a8772e Fix iframe HTML validation errors.
a714afcc Fix table related HTML validation errors.
7bbb7823 Fix IE and minor tweaks (#2392)
...

@XhmikosR
Copy link
Contributor Author

Cool, just need another approval and remember, do not squash :)

@XhmikosR
Copy link
Contributor Author

BTW this PR uses node-sass. We could look into using dart-sass in another PR which might save us quite a few devDependencies.

Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants