Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Class and tag update for sidebar filter title #1280

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Member

iloveitaly commented Mar 16, 2012

Looks like spree is using h6 as the standard header tag for taxonomies and different sidebar titles. Switching from span.category makes styling a bit easier.

Owner

schof commented Mar 16, 2012

@devilcoders: you ok with this change?

Member

jumph4x commented Mar 16, 2012

Makes sense semantically, assuming we have an h1..h5 present already.

Either way, messing with markup semantics for ease of styling is incorrect. Should be the other way around. Styling follows semantics. These things have repercussions the more spiders pay attention to the structure of the content.

Member

devilcoders commented Mar 17, 2012

@schof Yes, I guess I just missed this piece. But we need to make sure this doesn't beak the styling of that title. Will check this on monday morning.

Member

iloveitaly commented Mar 18, 2012

@jumph4x Styling should definitely follow markup, not the other way around. The markup should be structured in a way that reflects the importance of the content itself and should fit together with the conventions used by the the rest of the structure in a given application. Am I off anywhere here? Would there be a better way to handle the above markup change?

Thanks

I don't think it's right to use an h6 heading without having an h5 parent > h4 parent > h3 parent > h2 parent > h1 parent. It doesn't make sense, and you shouldn't skip headings. Using the h6 just because it's less important isn't semantically correct. The hierarchy is what determines the heading level. (see here)

Member

iloveitaly commented Mar 19, 2012

@jtwalters What would be the best solution here then? It seemed as though some spree components took the approach that choosing h6 was a safe bet. I'm guessing the logic here was it shouldn't be up to Spree to use all headers less than h6 on a page – this is up to the developer. Given the number of extensions available and customizations that most stores will apply to product pages and product listing, this seems to make sense to me albeit not a perfect solution considering the possible gap in header hierarchy.

Member

jumph4x commented Mar 19, 2012

Perfect solution is to leave as is: a span element. a semantically weightless container used simply for its CSS selecting ability.

Instead create a representing abstracted CSS class like 'faux-sidebar-heading' and change the default stylesheet like so:

h6{ property: value} ->

h6,
.faux-sidebar-heading{ property: value}

I don't know if there is a good solution to the heading hierarchy issue. It's difficult to maintain correct heading hierarchies when multiple views and partials contribute to the heading hierarchy. Overall, I would say there is much room for improvement with the semantic use of heading elements.

Here's some related issues we should think about:

  • The homepage doesn't appear to have any headings. There should be an h1.
  • Taxonomy product listings skip to h3 (breadcrumb leaf) when that should probably be the h1. Sidebar uses h6 "Shop by Brand", "Shop by Category" etc.
  • Cart has an h1, but uses h4 for product titles (skips h2, h3)

I agree with jumph4x's solution to use a span element with class instead. It's not as terse as h6 without a class, but as previously mentioned, "styling follows semantics."

Member

devilcoders commented Mar 19, 2012

Let me explain the reason why we have span there. The hardest part of writing new base theme was to make it with less possible structure changes. So, if you have custom overrides or even old hooks, whatever, it will work out of the box. Some parts that can go without any structure changes and fit into new layouts are left unchanged. I agree that semantically the headers would be logically here. I even test this, and write some css to make it work today. I think this will be accepted soon with minor corrections.

Owner

schof commented Mar 22, 2012

Cool. Let's make sure to close this ticket once those changes are merged in.

Member

radar commented May 24, 2012

Any updates on this one guys?

Member

radar commented May 30, 2012

This is now fixed with #1613 in 1-1-stable.

@radar radar closed this May 30, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment