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

Add blank lines in print css and fix block comments. #31

Merged

Conversation

Jeroen-Matthijssens
Copy link
Contributor

css files leave a blank line in between every block. But in _print.css this is not the case, which makes is less readable. The block comments in this file have extra whitespace at the beginning of consecutive lines which has been removed.

@coliff coliff changed the title Add blank lines in prent css and fix block comments. Add blank lines in print css and fix block comments. Jun 17, 2019
@coliff
Copy link
Member

coliff commented Jun 17, 2019

Hey there - thanks for the PR! You're right - there should be a blank line between each rule before each rule - but I don't think a blank line is needed on line 8 (before the first rule) or 75 (after the last rule).
could you update that and run the build script so the dist is updated?

@Jeroen-Matthijssens
Copy link
Contributor Author

I don't know what the policies are about updating the pull request but I figured that just force pushing seems the cleanest.

@coliff
Copy link
Member

coliff commented Jun 18, 2019

thanks for updating that - the dist folder needs to be updated too though.

@Jeroen-Matthijssens
Copy link
Contributor Author

Apparently gulp 3 and node 12 don't go together. I had to resort to some docker trickery in order to run the build process. But it is done. :)

@coliff
Copy link
Member

coliff commented Jun 18, 2019

Apparently gulp 3 and node 12 don't go together.

Interesting! Maybe it's time we looked to update the script to Gulp 4.

Copy link
Member

@coliff coliff left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Jeroen-Matthijssens
Copy link
Contributor Author

I got a wierd error and ended up on [1], I did not look into it further, but using node 11.15.0 did just work.

[1] https://stackoverflow.com/questions/55921442/how-to-fix-referenceerror-primordials-is-not-defined-in-node

@roblarsen roblarsen merged commit 71d6079 into h5bp:master Jun 18, 2019
@roblarsen
Copy link
Member

Great! time to test out my limited Gulp skills!

@roblarsen
Copy link
Member

Also, thanks!

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

3 participants