Simpler markup css js experiments

wpaccessibility / settings-api-enhanced

An improved WordPress Settings API with default render callbacks and a new accessible layout.

9 stars 0 forks source link

Simpler markup css js experiments #16

Closed afercia closed 7 years ago

afercia commented 7 years ago

Opening this PR for discussion. I've experimented a bit trying to implement some CSS naming convention. Among other things, the js-targeting classes allow to greatly simplify the JS part. Also, tried to experiment some design details that look a bit more close to the original design by Michael Arestad Some experiments are marked with an experimental comment in the CSS. Not all of them should be implemented 🙂 for example the date/time custom format fields are (maybe) prettier but not so ideal for accessibility,

Of course, these are just experiments, worth discussing if some of the things tried here would add some value.

afercia commented 7 years ago

This PR also adds the Media Settings page.

felixarntz commented 7 years ago

I haven't had time to thoroughly review this, but I'd like to discuss the CSS naming conventions a bit more.

Observations / ideas:

Removing the settings- prefix is definitely the right decision, since this markup is not limited to settings pages.
You changed settings-field-control to control, but settings-field-title to field-title. It's strange to me that one has the field- prefix while the other hasn't.
I like the simplicity of the class names you chose, but I fear conflicts with other plugins (and maybe even Core itself). We should probably consider figuring out a base prefix we use for everything (wp- or wp-core- maybe?). While these short names are perfect if they were in their own project, I think in a huge environment with long history such a prefix is required.

afercia commented 7 years ago

You changed settings-field-control to control, but settings-field-title to field-title. It's strange to me that one has the field- prefix while the other hasn't.

Yeah, with BEM I'd probably do field__title but we can't use BEM...

I fear conflicts with other plugins (and maybe even Core itself).

Plugins should always use prefixes. As per a wp- prefix for core, I'd be in favor of it but that was already discussed on Slack and there was no consensus 🙂 https://wordpress.slack.com/archives/C02RQBWTW/p1491841961906001

afercia commented 7 years ago

This PR also adds the Media Settings page.

I think there are some fields that are displayed conditionally that I've missed.

afercia commented 7 years ago

Will split the Media page addition in a separate PR. Worth noting this PR includes a couple fixes that were recently merged in core and should be merged (here or separately) for parity. See:

Avoid a keyboard trap on the date and time custom format settings https://core.trac.wordpress.org/changeset/40568

Restore missing spinner when installing a new language. https://core.trac.wordpress.org/changeset/40579

felixarntz commented 7 years ago

Thanks for all the work you've put into this @afercia! I agree with most of the changes, but it's hard for me to tell which ones exactly. I think it would be better if we closed this PR and broke it up into smaller pieces for the individual items. Because of other changes, it now also produces a conflict, another indicator these smaller pieces are easier to handle. This PR just does too much :)

afercia commented 7 years ago

Yep this one was "experimental" and made sense only if we wanted to merge it soon. I've already split and merged the JS part in #26. I'd agree to close the PR but keep the branch as reference for some of the things we should experiment.

afercia commented 7 years ago

For reference, this is what the CSS part in this PR was trying to experiment: