Conversation
5d34494 to
7657e6b
Compare
| "./src/**/*.mdx", | ||
| "./src/**/*.md", | ||
| // Scan packages/js for Tailwind classes used in WordPress plugin components | ||
| "../js/src/**/*.js", |
There was a problem hiding this comment.
Not sure if adding path is safe. @yoast/ui-library is a distributed packaged available on NPM. Any external consumer of the package won't have access to../js/src/**/*.js, which will be problematic.
There was a problem hiding this comment.
😅 Right good point. I'll remove that and add the media query to the styles sheet.
…om:Yoast/wordpress-seo into jordi-pv/use-the-dialog-toast-in-wordpress
…cus-managment' of github.com:Yoast/wordpress-seo into jordi-pv/use-the-dialog-toast-in-wordpress
| id="yoast_wpseo_task_list_opt_in_notification" | ||
| isVisible={ isVisible } | ||
| className="yst-w-96" | ||
| className="yst-w-96 md:yst-left-[160px]" |
There was a problem hiding this comment.
So here we always pass left. Would this be working for RTL?
…log-toast-in-wordpress
Pull Request Test Coverage Report for Build c77444feb137db226807f53614dde17a23e5e54cDetails
💛 - Coveralls |
…se ModalNotification for improved accessibility and user experience
…log-toast-in-wordpress
| isOpen={ isOpen } | ||
| onClose={ onClose } | ||
| className={ classNames( | ||
| isAdminSidebarExpanded() && "md:yst-start-[160px] rtl:yst-end-[160px]" |
There was a problem hiding this comment.
Lets fix spacing in RTL and also try to stick to the tailwind classes when possible. For some reason the space in RTL doesn't include the 16px padding, I'm not sure why, but this is a work around.
| isAdminSidebarExpanded() && "md:yst-start-[160px] rtl:yst-end-[160px]" | |
| isAdminSidebarExpanded() && "md:yst-left-40 rtl:md:yst-right-44" |
There was a problem hiding this comment.
The rtl: variant is not configured in the Tailwind preset. See the comment in the Know limitations section:
RTL positioning: The ModalNotification sidebar offset does not fully work in RTL mode. The md:yst-start-[160px] Tailwind class is picked up because it already exists in the CSS bundle, but the RTL-specific class (rtl:yst-end-[160px]) is not generated because the ui-library's Tailwind config only scans its own src/ directory — arbitrary value classes used in packages/js are not included.
vraja-pro
left a comment
There was a problem hiding this comment.
CR & AC 🚧
Few comments for mobile and RTL.
| return <ModalNotification | ||
| isOpen={ isOpen } | ||
| onClose={ onClose } | ||
| className={ classNames( |
…index for improved visibility
…yout and improve sidebar responsiveness
| notificationPositionClass = "md:yst-start-[3.25rem]"; | ||
| } else { | ||
| notificationPositionClass = "md:yst-start-10"; | ||
| } |
There was a problem hiding this comment.
I had to this if statement because the linter complains if I add nested ternary operators.
const modalPositionClass = isAdminSidebarExpanded() ? "md:yst-start-40 rtl:md:yst-start-44" : isRtl ? "md:yst-start-[3.25rem]" : "md:yst-start-10";
…lacing 'left' and 'right' with 'start' and 'end' classes in Tailwind CSS.


Context
The TaskListOptInNotification component uses a
Toastwhich lacks focus trapping and proper modal accessibility. This PR replaces it with theModalNotificationcomponent (HeadlessUIDialog) for improved accessibility, focus management, and keyboard navigation. The notification is also correctly positioned to offset it from the WordPress admin sidebar.Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Toastcomponent withModalNotificationinTaskListOptInNotificationfor improved accessibility and focus management.ModalNotificationuses HeadlessUI'sDialog, providing focus trapping, Esc key dismissal, and proper ARIA semantics out of the box.OptInContainernow always rendersTaskListOptInNotificationand controls its visibility viaisOpen/onCloseprops (driven by route and notification-seen state), rather than conditionally mounting/unmounting the component.<OptInContainer />outside the<Notifications>wrapper inapp.jssinceModalNotificationrenders via its own HeadlessUI portal with fixed positioning.isAdminSidebarExpanded()helper to detect whether the WP admin sidebar is expandedTest instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Steps
Using a live-link site with Local by Flywheels
_yoast_wpseo_task_list_opt_in_notification_seenrow from the wp_usermetaLTR — Expanded sidebar
5. Verify the notification appears at the bottom-left of the screen, offset from the expanded WordPress admin sidebar, with a consistent gap.
LTR — Collapsed sidebar
6. Collapse the admin sidebar (click the collapse arrow at the bottom of the sidebar).
7. Remove the
_yoast_wpseo_task_list_opt_in_notification_seenrow again and reload.8. Verify the notification appears at the bottom-left, offset from the collapsed sidebar, with a consistent gap.
RTL — Expanded sidebar
9. Switch to RTL by appending
?d=rtlto the URL (or via "Switch to RTL" plugin).10. Remove the row again and reload.
11. Verify the notification appears at the bottom-right of the screen, offset from the expanded sidebar on the right side, with a consistent gap.
RTL — Collapsed sidebar
12. Collapse the admin sidebar.
13. Remove the row again and reload.
14. Verify the notification appears at the bottom-right, offset from the collapsed sidebar on the right side, with a consistent gap.
General checks
15. Resize the screen and confirm the notification is responsive and doesn't overlap the WP admin sidebar in any of the 4 states above.
16. Verify the close (
X) button is focusable when rendered (don't close it yet).17. Navigate with the Tab key and confirm that the focus is trapped within the notification buttons.
18. Confirm that you can't scroll the Yoast General admin page behind the notification.
19. Press the Esc key and confirm that the notification is dismissed.
20. Inspect the element with the dev tools.
21. Confirm the notification is rendered as a HeadlessUI
Dialogwithrole="dialog"and is a child of a portal element.22. Smoke test the Task-list introduction in different browsers.
Relevant test scenarios
Check the console for any React warnings or errors. Test on Chrome, Firefox, and Safari to ensure focus management works correctly across browsers.
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.[yoast-doc-extension], added test instructions for Yoast SEO for Google Docs and attached theGoogle Docs Add-onlabel to this PR.Documentation
Quality assurance
grunt build:imagesand commited the results, if my PR introduces new images or SVGs.Innovation
innovationlabel.Fixes #4372