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

Format SCSS #1168

Closed
wants to merge 1 commit into from
Closed

Format SCSS #1168

wants to merge 1 commit into from

Conversation

m-e-h
Copy link

@m-e-h m-e-h commented Jul 7, 2017

Changes proposed in this Pull Request:

Lint and properly format SCSS so it'll produce a properly formatted style.css

Related issue(s):

#1149
#1159
#932
#893

@m-e-h
Copy link
Author

m-e-h commented Jul 8, 2017

Oops. Looks like I need to squash a few commits here. Reckon I should add the compiled style.css too...

@m-e-h
Copy link
Author

m-e-h commented Jul 10, 2017

The updated style.css has a couple minor quirks due to node-sass and/or WP CSS coding standards.

  1. empty selector blocks for the menu section only appear in the SASS files.
  2. single line comments on the same line as a parameter have been changed from /* */ to //, so they only show in the SASS files, as well. This is because during compilation the /* */ was being moved to it's own line.

If automating the style.css generation through SASS compilation is the goal, I don't see many ways around making some adjustments like this.

I did still have to make one manual fix to the style.css.
I think it was node-sass that didn't seem to like this piece of code.

.comment-navigation,
.posts-navigation,
.post-navigation {

	.site-main & {
		margin: 0 0 1.5em;
		overflow: hidden;
	}
}

It produced this in the CSS

.site-main .comment-navigation,
.site-main
.posts-navigation,
.site-main 
.post-navigation {
	margin: 0 0 1.5em;
	overflow: hidden;
}

The SCSS will probably need adjusted here but I thought that could be done after this commit?
Please let me know any thoughts on these or anything else that doesn't look right.

@m-e-h
Copy link
Author

m-e-h commented Jul 10, 2017

Just noticed this has some overlap with this #1153 and this #1170

@grappler
Copy link
Collaborator

Thank you for the PR.

As #1149 has started to question the need for Sass we will need to wait to merge this till we have a resolution.

As we are planning to update Normalize in #1155 the pull request will need to be refresh afterwards.

@Ismail-elkorchi
Copy link
Contributor

The formatting issues of SASS and CSS were resolved in #1391

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.

3 participants