themehybrid / hybrid-core

Official repository for the Hybrid Core WordPress development framework.
GNU General Public License v2.0
689 stars 144 forks source link

All Elements Should Have a Class #71

Closed robneu closed 9 years ago

robneu commented 10 years ago

I've been lurking around for quite a long time now, but I haven't really been an active participant. Thanks for putting so much time and effort into Hybrid Core! It's really great. :)

One thing that I've noticed is that the implementation of the hybrid_attr() function has left a number of elements without a class of any kind. This forces developers to use IDs in their CSS which is less than ideal. I realize these can be added using a filter in the theme, but it seems like something that should probably be addressed in core.

The elements that jumped out at me were the site header and footer, but there may be more that I haven't noticed yet. If you think this is worth doing, I'd be happy to submit a PR with the changes.

m-e-h commented 10 years ago

I was thinking this same thing just last week but then I started playing with the attr filter and realized it didn't matter. I ended up overriding many of the existing classes anyway. Hybrid_attr() has changed my life :-) I do agree though, that encouraging the use of classes over IDs in css would make styling a child theme much easier.

justintadlock commented 10 years ago

It's OK by me if we add classes to elements without them.

I'm not sure why IDs are less than ideal. They do exist for a reason. I've also never had any trouble styling them from a child theme.

m-e-h commented 10 years ago

I think the class over id thinking mostly applys to large css projects with lots of developers. Though I have had to wrestle with some styled IDs a time or two. Classes are just easier to overwrite. http://codepen.io/chriscoyier/pen/lzjqh

robneu commented 10 years ago

IDs definitely do have a purpose. They're required for accessibility reasons and I understand they're also less resource-intensive for JS to find in the DOM. For CSS though, they lead to problems with specificity and make it difficult to override selectors without getting crazy weird. They also make it difficult to reuse blocks of styles in an object-oriented way because an ID can never be used more than once on the page.

Here's some reading on the subject:

http://csswizardry.com/2011/09/when-using-ids-can-be-a-pain-in-the-class/ http://screwlewse.com/2010/07/dont-use-id-selectors-in-css/ http://www.smashingmagazine.com/2011/12/12/an-introduction-to-object-oriented-css-oocss/

The benefit to adding the classes is that anyone who disagrees with any of this can continue to use IDs while people who do see the merit of writing CSS in this way can use classes. Everyone wins! :)

justintadlock commented 10 years ago

I understand the arguments for why styling with IDs can be a PITA, but I'm referring specifically to the things within Hybrid Core that need to be addressed. I'm sure there are some things. #header and #footer come to mind. But, I'm not sold on needing classes for #site-title and #site-description, for example.

Like I said though, this enhancement idea is fine by me.

justintadlock commented 9 years ago

I added classes where I found them missing in the dev branch. Pleas open new tickets for further changes.