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

Improve markup by removing unnecessary elements #32

Merged
merged 4 commits into from
Jun 8, 2017

Conversation

felixarntz
Copy link
Contributor

This PR addresses #22 and #23.

It does the following:

  • Remove the .settings-field-control span element and instead add this class to every field automatically (input, select, textarea, EVERY element).
  • Remove the .settings-sections div.
  • Adjust the forms.css file to the change with the .settings-field-control class, as it is now for the actual field, not its wrap element.
  • Improve forms.css file organization by grouping related rules under headings.
  • Remove unnecessary bloat from forms.css. I considered everything "unnecessary bloat" that was referenced elements on a specific page that don't have anything to do with the Settings API and the new markup. These things were previously in there from being copied from Core's original forms.css.
  • Ensure all labels in a radio/checkbox group also have the typical 16px font size that other inputs have. While those are labels, they visually belong to the input, so they should have the same style.
  • Set the font-weight of a single checkbox label to not be in bold any longer. This was likely overlooked (because in current Core all these labels are bold), and it was weird that the label of a single checkbox was previously the only bold text on a settings page (except for the page heading).

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.

@felixarntz felixarntz added this to the WCEU-Prototype milestone Jun 4, 2017
@joyously
Copy link

joyously commented Jun 4, 2017

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
Copy link
Contributor

afercia commented Jun 4, 2017

@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
Copy link
Contributor

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

@felixarntz
Copy link
Contributor Author

@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
Copy link
Contributor

afercia commented Jun 7, 2017

@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
Copy link
Contributor Author

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

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.

4 participants