wpaccessibility / settings-api-enhanced

An improved WordPress Settings API with default render callbacks and a new accessible layout.
9 stars 0 forks source link

Improve markup by removing unnecessary elements #32

Closed felixarntz closed 7 years ago

felixarntz commented 7 years ago

This PR addresses #22 and #23.

It does the following:

Of course these CSS changes should not be considered final, but they provide a better base to work from. I didn't adjust any class names here, as that's part of another discussion likely not getting ready before WCEU.

@afercia Can you review the markup changes? This is probably the easy part :)

And @karmatosed, can you do a quick review on the CSS changes? Should be fine, but minor tweaks might be necessary.

joyously commented 7 years ago

It should probably be a different PR, but I think setting font sizes in px is a trend that should be fixed in this overall rewrite. One of the main reasons to avoid px is accessibility concerns. This is an old page, but succint: Using Font Size.

afercia commented 7 years ago

@felixarntz looks good to me except for one thing. Not sure why we need 2 wrappers for each section. settings-fields looks not necessary to me. It's just an inner div inside the section, and seems it's used just for styling. In #16 I was able to have just one wrapper.

karmatosed commented 7 years ago

@felixarntz I 'think' its good. Lets maybe get in and I can do a pass tomorrow on visuals too. Thanks.

felixarntz commented 7 years ago

@afercia Being used for styling is a valid reason to keep it IMO. It should be possible to target for example .settings-field:first-child which wouldn't work if we removed the wrap element. A div doesn't have semantic meaning, so I don't see an issue with it being there for styling only. A .settings-section may contain more than just the fields (heading, description, whatever gets printed in a callback), so I think this wrap element is necessary.

Furthermore, if we wanna get into semantics, let's look at lists for example. There's always a ul wrapping the lis. This makes sense for both semantics and to be able to style them. We could even think about making the .settings-fields element a ul and the .settings-field elements lis (although that's too late for now and should happen in a later PR).

afercia commented 7 years ago

@felixarntz on the first point we clearly have a different view :) Not so important for now, however it's not about semantics, it's about clean code and divitis. I think at this stage we shouldn't really be too worried by CSS concerns, that can be addressed later. Anyways, let's move on 😉

About the second point (lists) I'd say no. Forms already have all the semantics users need and users expect they're just... forms. When there's a fieldset, screen readers already announce the number of items in the fieldset. Also, when inside a form, screen readers switch to "forms mode": and tend to read out only the form fields and what is associated to form fields (e.g. an element targeted by aria-describedby) ignoring other elements.

felixarntz commented 7 years ago

I'll merge this now as is. I opened #37 to continue discussion whether we should remove .settings-fields as well.