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

Issue-125: Update UX of Settings Page #136

Merged
merged 1 commit into from
Mar 5, 2025
Merged

Conversation

samhermes
Copy link

Summary

Fixes #125

The display issues with repeatable fields on the settings page have been resolved, and anchor tags have been swapped out in favor of button elements where appropriate. The use of jQuery on the settings page has also been refactored out.

Changes Made

  • Spacing adjusted around repeatable field groups
  • Removed jQuery dependency from the settings page

Testing Instructions

  1. Navigate to the settings page (Settings > SEO).
  2. Verify that all functionalities on the page work without errors.
  3. Navigate to an "Add New" taxonomy term page.
  4. Verify that the character counts work when adding a new term.
  5. Navigate to the post editor screen on a post type where the plugin is enabled.
  6. Verify that the character counts work when updating the title or description in the SEO settings.

@mboynes
Copy link
Contributor

mboynes commented Feb 26, 2025

A couple of general notes/questions:

  • Since the script still depends on jquery, it feels like this didn't achieve one of the primary objectives.
  • While not part of the "AC", inasmuch as there was any, it would have been nice to also get rid of the underscores dependency.
  • Why not leverage React? Seems like this could have been much simpler with React.
  • Also not a part of the "AC", but the scripts and styles are enqueued on every admin request, it would be good to use this as an opportunity to reign that in.

Copy link

@mogmarsh mogmarsh left a comment

Choose a reason for hiding this comment

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

👍

});

// Update the character counts after a term is added via AJAX.
jQuery(document).ajaxComplete(function () {
Copy link

Choose a reason for hiding this comment

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

I'm guessing we're stuck with this one because core is using it?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I've looked at alternate ways of handling this, and there just isn't a good option. The form gets submitted by jQuery, and we need to listen to the jQuery event for the submission, it's not available in vanilla JS.

Copy link

Choose a reason for hiding this comment

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

totally makes sense. ship it

@samhermes
Copy link
Author

@mboynes I created #139 for tracking the scripts and styles enqueue updates. We need to remove the standard meta fields from the block editor, since it's being replaced by the slotfill, then we can avoid enqueuing the script there.

Agreed about React, it would have made this much easier, and made it easy to get rid of the Underscores dependency as well.

@samhermes samhermes merged commit 5d8e501 into v2.0 Mar 5, 2025
6 checks passed
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.

3 participants