-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
Conversation
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 |
This can be merged after #2625 and after a rebase. |
@nodejs/website |
Here's the latest diff of the generated unminified CSS https://gist.github.com/XhmikosR/10aaa5ea030adfc87838daad111335e4/revisions |
Great job! 💯 Changes looks okey at a glance. GitHub's diff view gives the impression I would think
Any thoughts? |
..sorry, I obviously pressed the wrong button while posting the comment above. |
I think this happens because the files contain a lot of changes, so they are basically new. |
I actually tried this with git mv and it's the same. The files have many changes that git considers them as new. |
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 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 |
That would mean we'd have a broken build in the first commit, though, right? We don't do much |
Yes, indeed the first commit would cause havoc on its own. Unless we can
configure stylus to use a different file extension temporarily. But then it
starts to be quite the detour just for the sake of preserving git history 😬
Would you consider it worthwhile Rich? Or should we just forget about it?
…On Sat, 12 Oct 2019 at 06:16, Rich Trott ***@***.***> wrote:
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?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#2530?email_source=notifications&email_token=AAJMWE7VRKKSTNBQ5FXZCU3QOFFSXA5CNFSM4IUQSQYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBBVEXQ#issuecomment-541282910>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMWE6OVRTDFMP2TBXEOFLQOFFSXANCNFSM4IUQSQYA>
.
|
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
I fixed a couple more issues I noticed, namely the server watch for scss files and README.md and TRANSLATION.md references to stylus |
Looks great! Although GitHub's UI suggests the old files are deleted and completely new ones are added, it works as expected locally;
|
Cool, just need another approval and remember, do not squash :) |
BTW this PR uses node-sass. We could look into using dart-sass in another PR which might save us quite a few devDependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I open this now, but requires some of my other PRs merged.
I will split the output folder change to a separate PR.See #2544Switch to Sass.
Also:
DO NOT SQUASH