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

Using WordPress Style Standards #286

Closed

Conversation

BFTrick
Copy link
Contributor

@BFTrick BFTrick commented Aug 17, 2013

We should update the style.css file to use the WordPress CSS Coding Standards.

@emiluzelac
Copy link
Contributor

It's right here: https://github.com/emiluzelac/_s/commit/3a02e91a7475667b4beb3d6299e390462ae6e987 but for some reason it was deleted from _s commit.

@BFTrick
Copy link
Contributor Author

BFTrick commented Aug 20, 2013

Hi @emiluzelac,

I don't see that commit in the _s history at all. It doesn't look like you ever submitted a pull request??

I added your spacing suggestion (since they are in the WordPress standards) to this commit. :)

@emiluzelac
Copy link
Contributor

Thanks bud, I appreciate that. Not really sure what happened to pull request. But, we got it now :)

@BFTrick
Copy link
Contributor Author

BFTrick commented Aug 20, 2013

Collaboration. 👍

@emiluzelac
Copy link
Contributor

👍

@emiluzelac
Copy link
Contributor

Can we get this as well please? Not much but it's correct
https://github.com/emiluzelac/_s/tree/patch-3
On Aug 19, 2013 10:58 PM, "Patrick Rauland" [email protected]
wrote:

Collaboration. [image: 👍]


Reply to this email directly or view it on GitHubhttps://github.com//pull/286#issuecomment-22921021
.

@BFTrick
Copy link
Contributor Author

BFTrick commented Aug 20, 2013

@emiluzelac I added a separate pull request for that since it seems like a different issue. That way we can keep this one on topic and easy to pull in for admins. :)

@emiluzelac
Copy link
Contributor

Thanks
On Aug 20, 2013 7:30 AM, "Patrick Rauland" [email protected] wrote:

@emiluzelac https://github.com/emiluzelac I added a separate pull
request #290 for that since it
seems like a different issue. That way we can keep this one on topic and
easy to pull in for admins. :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/286#issuecomment-22941311
.

@sheabunge
Copy link
Contributor

👎 I don't like the idea of making the stylesheet longer by splitting selectors into different lines. This should only happen when the selectors are too long or complex to group together on a line.

@mfields
Copy link
Contributor

mfields commented Aug 20, 2013

I agree with @bungeshea the single line selectors are short and easy to read. Expanding a reset like this is a bit distracting to anyone reading the stylesheet's source. Suggest close.

@emiluzelac
Copy link
Contributor

That's not the case, nothing is made longer. All is still the same, I just made the same consistency, that is all.

@philiparthurmoore
Copy link
Collaborator

I feel like this is very much a case of preference. I for one prefer multi-line selectors in everything that I do, even resets. It's how I'm able to gut out old selectors that aren't needed anymore and not lose track of what's being reset.

@emiluzelac
Copy link
Contributor

Agreed and hey, it's all up to you guys :)

@philiparthurmoore
Copy link
Collaborator

I'm going to close this out for now because I feel like there are bigger fish to fry. I will say for the record though that I absolutely, 100% agree that everything should be on its own line and maintain my own codebases in that manner. unfortunately I feel like that may be somewhat of a hard sell for most developers who use _s to build themes, so let's close that out and let others maintain their own style guidelines based on forked versions of _s.

@emiluzelac
Copy link
Contributor

No problem.

CostyEffe pushed a commit to silverbackstudio/wp-theme that referenced this pull request Jun 23, 2020
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.

5 participants