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

Updated settings demo page #145

Merged
merged 7 commits into from
Mar 15, 2023

Conversation

erikyo
Copy link
Collaborator

@erikyo erikyo commented Jan 20, 2023

What?

Updates the settings page demo code

Why?

Accessibility: settings checkbox labels are not unique #110

How?

The settings table has been refactored, now it is built by a couple of loops and the ids are dynamically generated so there should be no more duplicate ones

Testing Instructions

@erikyo erikyo requested review from bacoords and Sephsekla January 20, 2023 12:58
includes/class-wp-notify-demo.php Outdated Show resolved Hide resolved
*
* Development demo files
**/

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's something I'm planning on taking on as a larger project, but what do you think about using a namespace for this instead of prefixes?
Probably

namespace WP\Notifications\Demo;
use WP_Admin_Bar;

etc

@bacoords
Copy link
Collaborator

This looks good. A few random thoughts here:

  • We'll need a way for plugins/themes to register or hook into this table to add their own rows- perhaps based on a notification source/category or else
  • I sure would love to see an example of a new WP settings page built with JS components instead of WP List Table (even though the WP List table here is a big improvement over the previous static HTML). Is there anything in core using JS for admin/settings areas outside the block/site editor?
  • This screen is missing some sort of "save" button

@erikyo
Copy link
Collaborator Author

erikyo commented Feb 19, 2023

1 - About the first thing (giving plugins the possibility of adding fields) I had thought about it. We just add a filter here to add a row but it is a bit more complicated to add a column... it is not impossible but at the moment I did not want to create much code for it as we still have to define it well (see next point)

2 - As a great supporter of "js for everything" I would say no because it would be completely different to the standard WordPress "way", and besides I don't see the need. Anyway this is a possibility I have explored already in this repo where I made a settings panel that saves using the rest-api. I thing that could be nice for a plugin with a lot of settings (like woocommerce) but in our case could be an unnecessary overhead.

3 - 🤦the table does not provide a confirmation button because it is not a form. this could fix

includes/demo.php Outdated Show resolved Hide resolved
@erikyo erikyo requested a review from Sephsekla March 13, 2023 18:12
@Sephsekla Sephsekla added [Scope] User Interface For displaying to and interacting with end users. demo A11y Accessibility bugs or enhancements labels Mar 14, 2023
@Sephsekla
Copy link
Collaborator

I think your suggestions make sense. Could you update this with the latest develop branch and merge your changes? We can iterate on it later but for now I'd like to get it done so we can unblock the e2e tests.

@erikyo erikyo merged commit 680be57 into WordPress:develop Mar 15, 2023
@erikyo erikyo deleted the enhancement/settings-page branch March 26, 2023 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A11y Accessibility bugs or enhancements demo [Scope] User Interface For displaying to and interacting with end users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants