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

Merge diff between sass and css #932

Closed
wants to merge 14 commits into from

Conversation

youthkee
Copy link

@youthkee youthkee commented Mar 3, 2016

When compiling Sass to CSS, there are some difference between both sources.
So, I merged to resolve conflicts.

@sixhours
Copy link
Contributor

I noticed this removes some of the un-styled selectors for .main-navigation from style.css; not sure that's a deal-breaker, but having those selectors readily available is handy if you're not using Sass.

@youthkee
Copy link
Author

It's true that there are un-styled selectors in style.css and _menus.scss both.
So I put back those selectors to style.css in places that correpond to _menus.scss.

@MarkBatchelder
Copy link
Contributor

I like the idea of reordering style.css to match the compiled SASS, but you removed some of the blank lines between selectors that should be put back before this is merged.
9a08ca0

Also the explanation mark is really unnecessary in style.css and has been debated in the past.
/*!

@youthkee
Copy link
Author

Thank you for your information.
I Added blank line between selectors to style.css, and put back the explanation marks.

@MarkBatchelder
Copy link
Contributor

And sorry, but to clarify style.scss requires the exclamation mark to keep that comment from being stripped when compiled, but isn't needed in style.css, although I don't know that it hurts anything in style.css. Also, I'm no expert here, I've just been looking over the CSS a lot myself.

@youthkee
Copy link
Author

I checked for the exclamation mark again, and I found it's necessary in style.scss to remain the comment section when compiling.
So, I put back this one.
e9c7d73

@joshmcrty
Copy link
Contributor

I saw this PR and was wondering, has anyone looked into compiling the sass on underscores.me instead and removing the CSS file from this repo? The intent is that developers generally download the theme from underscores.me and not directly from this repo, correct? It already handles find/replace for theme names, couldn't it handle creating the CSS file if you don't check the "sassify" box on underscores.me? That would eliminate some of the out-of-sync issues this repo has had in the past with CSS/Sass pull requests.

@grappler
Copy link
Collaborator

grappler commented Jul 7, 2017

@joshmcrty #1159 #1149 is looking at automating the CSS generation which may fix the out of sync issue without needing to generate the CSS on theme generation.

@m-e-h m-e-h mentioned this pull request Jul 7, 2017
@davidakennedy
Copy link
Contributor

Is this still an issue? When I ran a test compile yesterday, the only differences I saw were stylistic ones because of the Sass expanded output.

…s-diff

* automatic/origin/master: (193 commits)
  Update README.md
  Update template hierarchy link
  Update year to the latest
  Remove extra `!` in `style.scss`
  Remove unnecessary !
  Update screen-reader-text class.
  Fixing "Upload Theme" button text
  Add necessay backticks
  Change links http:// to https://
  Added a note about the need to rename `_s.pot`.
  Travis: build against highest available PHP version
  Remove CSS rules for quotes - Issue Automattic#1192
  Remove unused CSS.
  New lines and spacing.
  File must end with a newline character.
  Load WooCommerce Star fonts via inline styles.
  Adds WooCommerce plugin file paths.
  Fix column-width sass mixin.
  Add commas to each array value.
  Newline character.
  ...
@youthkee
Copy link
Author

youthkee commented Oct 31, 2017

I tried to compile with latest codes, and there seems to be one difference at a comment line break yet. Is it better to fix that?

I compiled with node-sass and used options below.

$ node-sass --output-style expanded --indent-type tab --indent-width 1 --precision 10 sass/style.scss style.css

@Ismail-elkorchi
Copy link
Contributor

Closing as this issue is addressed in #1391.

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.

7 participants