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

Adjusted labels for clarity and accessibility. #52

Merged
merged 10 commits into from
Oct 5, 2021
Merged

Adjusted labels for clarity and accessibility. #52

merged 10 commits into from
Oct 5, 2021

Conversation

soup-bowl
Copy link
Owner

Issue #51 raised by @kebbet highlights an important oversight that doesn't follow the standard of the rest of the WordPress administration dashboard, being that not only are labels missing from checkboxes, but the label-for in the settings key column were left undefined.

This PR adjusts the generation functions to make them less of a catch-all approach, and also allows the group of sporadic check boxes to be group together. The PR affects both single-site and multisite settings equally.

@kebbet if you wouldn't mind verifying if this is what you were expecting, especially as it adds more labels that will ultimately need translating and I don't want to add unnecessary burden if it's wrong.

Settings Checkbox Group

Before
image
After
image

Additionals

image
image

Additional adjustments

  • Multisite over-ride on settings generation now an inherited trait rather than a function argument.
  • Debug setting source indicators use the badges from mail view to help isolate their presences.

@soup-bowl soup-bowl self-assigned this Oct 2, 2021
@kebbet
Copy link
Contributor

kebbet commented Oct 2, 2021

Thanks for doing this! Will have a look and do a test run if I have the time during next week!

Copy link
Contributor

@kebbet kebbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just read the code, have not tested it, don't know how to fetch a pull request... 😂

src/settings/class-settings.php Show resolved Hide resolved
src/settings/class-settings.php Outdated Show resolved Hide resolved
src/settings/class-settings.php Outdated Show resolved Hide resolved
src/settings/class-settings.php Outdated Show resolved Hide resolved
src/settings/class-settings.php Outdated Show resolved Hide resolved
src/settings/class-settings.php Outdated Show resolved Hide resolved
src/settings/class-singular.php Outdated Show resolved Hide resolved
src/settings/class-singular.php Outdated Show resolved Hide resolved
src/settings/class-multisite.php Outdated Show resolved Hide resolved
@soup-bowl
Copy link
Owner Author

soup-bowl commented Oct 4, 2021

@kebbet Appreciate that one of your main points is that description should be used to identify the optional under-field info instead of the text alongside a checkbox. Now you've highlighted it, I've used subtext completely wrong by definition anyway.

Barring any better words, how about fieldside_text for the checkbox text variable name?

@kebbet
Copy link
Contributor

kebbet commented Oct 4, 2021

@kebbet Appreciate that one of your main points is that description should be used to identify the optional under-field info instead of the text alongside a checkbox. Now you've highlighted it, I've used subtext completely wrong by definition anyway.

Barring any better words, how about fieldside_text for the checkbox text variable name?

Much clearer with fieldside_text.

@soup-bowl
Copy link
Owner Author

Side note, I recently hooked up my Visual Studio Code to GitHub, and all your comments have appeared directly in my editor. Super neat!

@kebbet
Copy link
Contributor

kebbet commented Oct 4, 2021

Side note, I recently hooked up my Visual Studio Code to GitHub, and all your comments have appeared directly in my editor. Super neat!

Nice, I tried it too and can now see PR's and comments! Thanks!
I have integrated Bitbucket in VSC before, which works nicely!

@soup-bowl
Copy link
Owner Author

Apologies, I clicked the re-review button I'm not sure it did anything? Made changes, if you're happy I can merge this in.

Only uncommented thing is class-multisite.php:86, the multisite definition of email disabling is different to reflect disabling all multisite emails.

Copy link
Contributor

@kebbet kebbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small things. Looks nice overall!

src/settings/class-multisite.php Outdated Show resolved Hide resolved
src/settings/class-singular.php Outdated Show resolved Hide resolved
src/settings/class-singular.php Outdated Show resolved Hide resolved
src/settings/class-settings.php Outdated Show resolved Hide resolved
@soup-bowl soup-bowl changed the title Adjust labels (primarily checkboxes) for clarity and accessibility. Adjusted labels for clarity and accessibility. Oct 5, 2021
@soup-bowl soup-bowl linked an issue Oct 5, 2021 that may be closed by this pull request
@soup-bowl
Copy link
Owner Author

Fab - will merge in. Thanks for all your help! 😄

@soup-bowl soup-bowl merged commit 4540305 into main Oct 5, 2021
@soup-bowl soup-bowl deleted the issu56 branch October 5, 2021 16:54
This pull request was closed.
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.

Checkboxes missing labels
2 participants