Creates the popover component by Jordi-PV · Pull Request #22170 · Yoast/wordpress-seo

Jordi-PV · 2025-04-03T14:25:26Z

Context

We want to add a reusable popover component to the Yoast ui-library.

Summary

This PR can be summarized in the following changelog entry:

Adds a popover component to the Yoast UI library.

Relevant technical choices:

I initially experimented with the native HTML Popover API, but ultimately decided to build our own component due to several limitations. For instance, browser support, the Popover api is not fully support by all browsers particularly for the popoverTargetElement feature (Can I use, browser support overview).
Additionally, the Popover API comes with built-in behaviors which required extra effort to override in order to gain full control. Customizing styles (e.g., adding pseudo-elements) also proved to be quite challenging, at least when grabbed in our .yst-root variables from the @yoast/components monorepo stylesheet dependency and displayed in Storybook.
The popover component is a state controlled component (meaning the parent component (ie button) owns the visibility state and decides when the popover should be shown.
The style of the popover should match this design.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Checkout to this branch in the wordpress-seo repo.
In your terminal run: yarn workspace @yoast/ui-library storybook to open the storybook on your local server.
Under the Elements section on the left sidebar, look for the Popover component.
Test the different position of the popover by changing the position in the select menu options.
Try adding a different text by filling in the Edit string... field in the children argument.
Toggle the backdrop to see the popover with a backdrop.

Relevant test scenarios

Changes should be tested with the browser console open
Changes should be tested on different browsers
Changes should be tested on multisite

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

UI changes

This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

I have tested this code to the best of my abilities.
During testing, I had activated all plugins that Yoast SEO provides integrations for.
I have added unit tests to verify the code works as intended.
If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
I have written this PR in accordance with my team's definition of done.
I have checked that the base branch is correctly set.

Innovation

No innovation project is applicable for this PR.
This PR falls under an innovation project. I have attached the innovation label.
I have added my hours to the WBSO document.

Fixes #22165

coveralls · 2025-04-03T14:41:41Z

Pull Request Test Coverage Report for Build e10e1cd542012d9b5fa007ab0e7e5d20fcd9487c

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
2463 unchanged lines in 209 files lost coverage.
Overall coverage decreased (-1.9%) to 52.552%

Files with Coverage Reduction	New Missed Lines	%
admin/class-admin-init.php	1	0.0%
admin/class-option-tabs-formatter.php	1	0.0%
packages/js/src/installation-success.js	1	66.67%
src/actions/importing/aioseo/aioseo-posts-importing-action.php	1	46.51%
src/config/conflicting-plugins.php	1	0.0%
src/config/migrations/20201216124002_ExpandIndexableIDColumnLengths.php	1	0.0%
src/config/migrations/20201216141134_ExpandPrimaryTermIDColumnLengths.php	1	0.0%
src/dashboard/domain/content-types/content-types-list.php	1	15.38%
src/dashboard/infrastructure/taxonomies/taxonomies-collector.php	1	0.0%
src/dashboard/user-interface/configuration/site-kit-configuration-dismissal-route.php	1	84.62%

Totals
Change from base Build 96c815bb673790099c1ce98fee6d4773cfcb1fb4:	-1.9%
Covered Lines:	29257
Relevant Lines:	56785

💛 - Coveralls

…ules.

igorschoester

I can't help but look at UI library PRs 😁
Not a 100% thorough review, but noticed quite some things anyway 😇

Also, this might give you some inspiration: https://www.radix-ui.com/primitives/docs/components/popover

igorschoester · 2025-04-15T09:42:37Z

+			<button
+				type="button"
+				onClick={ handleDismiss }
+				className="yst-bg-transparent yst-rounded-md yst-inline-flex yst-text-slate-400 hover:yst-text-slate-500 focus:yst-outline-none focus:yst-ring-2 focus:yst-ring-offset-2 focus:yst-ring-primary-500"


Some line stuff in Chrome:

Now the spacing is gone? So that is a UX question?

I've used the same style rules as the Modal.

igorschoester · 2025-04-15T10:08:20Z

With these multiple components and context I wonder if this is still a simple Element. Perhaps the context alone warrants a Component (sorry Toast).

https://ui-library.yoast.com/?path=/docs/introduction--docs#components

Indeed, the Toast example made me decide to consider it as an element instead of a component, but probably it is more a component. I'll wait to see if it ends up being a component (with multiple component and context) or it gets simplified into a single element, before moving it.

Reminder 😉

It has become a component 🙂

igorschoester

Another barrage of comments from me 😇
Left a few reminders on the old ones (be sure to click the expand on the collapsed comments?)

igorschoester · 2025-05-08T11:30:54Z

+			leaveFrom="yst-bg-opacity-75"
+			leaveTo="yst-bg-opacity-0"
+		>
+			<div className={ classNames( "yst-popover-backdrop", className ) } />


Can't we just use the Transition as a div?

<Transition as="div" show={ isVisible } appear={ true } unmount={ true } enter={ classNames( "yst-popover-backdrop yst-transition yst-duration-50 yst-ease-in", className ) } enterFrom="yst-bg-opacity-0" enterTo="yst-bg-opacity-75" entered={ classNames( "yst-popover-backdrop", className ) } leave={ classNames( "yst-popover-backdrop yst-transition yst-duration-50 yst-ease-in", className ) } leaveFrom="yst-bg-opacity-75" leaveTo="yst-bg-opacity-0" />

igorschoester · 2025-05-08T11:34:54Z

+	useEffect( () => {
+		if ( isVisible ) {
+			document.body.classList.add( "backdrop-active" );
+		} else {
+			document.body.classList.remove( "backdrop-active" );
+		}
+	}, [ isVisible ] );


Why is this needed?

It seems very unwanted to me due to:

document.body usage

no prefix and polluting outside of our "yst-root" scope

My bad once more! I needed to block mouse events behind the backdrop for the AI Optimize introduction in Classic Editor, but I shouldn't handle that this way 😓

igorschoester · 2025-05-08T11:37:57Z

+	isVisible,
+	setIsVisible,
+	position,
+	backdrop,


We usually prefix with something like is to indicate this is a boolean and not the thing itself (like your isVisible).
I would suggest hasBackdrop?

igorschoester · 2025-05-08T11:42:15Z

+Popover.Title = Title;
+Popover.CloseButton = CloseButton;
+Popover.Content = Content;
+Popover.Backdrop = Backdrop;


Since we expose the other components like this. We override the displayName in other components to prefix with the main name, see the Modal as example.

E.g. Backdrop.displayName = "Popover.Backdrop";

Sorry, but I don't see that being applied in the Modal (example) 🤔

Modal.Panel = Panel; Modal.Title = Title; Modal.CloseButton = CloseButton; Modal.Description = Dialog.Description; Modal.Description.displayName = "Modal.Description"; Modal.Container = Container;

The same as with the previously mentioned redundant displayName, it is also being used in the Modal.
Modal.displayName = "Modal";

igorschoester · 2025-05-08T11:42:35Z

+	);
+} );
+
+Popover.displayName = "Popover";


This seems redundant.

Why redundant, because it is named the same as the component? As far as I know it serves to properly name to the component for debugging purposes and, looking at other components in the ui-library (i.e. Modal, Card, TextareaField...), we also use the exact same name as the functions.

igorschoester · 2025-05-08T14:05:52Z

+			}
+		}
+
+		.yst-popover--top {


The very fine line that divides the arrow and the tooltip itself should probably not be there?

igorschoester · 2025-05-08T14:08:13Z

+			<button
+				type="button"
+				onClick={ handleDismiss }
+				className="yst-bg-transparent yst-rounded-md yst-inline-flex yst-text-slate-400 hover:yst-text-slate-500 focus:yst-outline-none focus:yst-ring-2 focus:yst-ring-offset-2 focus:yst-ring-primary-500"


Now the spacing is gone? So that is a UX question?

igorschoester · 2025-05-08T14:08:58Z

Reminder 😉

igorschoester · 2025-05-08T14:10:06Z

+			yst-flex
+			yst-self-start;
+
+			& button {


Why the &? 🤔

igorschoester · 2025-05-08T14:12:13Z

+export default {
+	title: "1) Elements/Popover",
+	component: Popover,
+	argTypes: {


Reminder 😉

mhkuu · 2025-05-12T14:12:51Z

I will need to merge this PR to be able to prepare the 25.2 RC. Further changes will have to go into a new PR.

Jordi-PV

If I am not mistaken, I've addressed all your feedback comments. The adjustments are made in 22260-adjust-the-popover-component

Adds the popover component

1f83585

Jordi-PV requested a review from a team as a code owner April 3, 2025 14:25

Jordi-PV linked an issue Apr 3, 2025 that may be closed by this pull request

Creates a popover component in the UI-library #22165

Closed

Jordi-PV marked this pull request as draft April 3, 2025 14:25

Jordi-PV added 6 commits April 4, 2025 16:36

Adds a context, title and close button to the component.

39e06cd

Adjusts the styling and the stories

d653b42

Adjusts the styling and the stories

4459908

Small adjustments

ba3288c

Refine component and story

bf5216e

Refine styles and stories

ce24282

Jordi-PV added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Apr 14, 2025

Jordi-PV added this to the 25.0 milestone Apr 14, 2025

Improves control management, adds accessibility props and rtl style r…

b69495a

…ules.

Jordi-PV marked this pull request as ready for review April 15, 2025 07:25

igorschoester reviewed Apr 15, 2025

View reviewed changes

marinakoleva removed this from the 25.0 milestone Apr 15, 2025

Jordi-PV added 6 commits April 16, 2025 16:02

Adjusts component after review

3a6e22b

Adjusts styling and markdown

468604a

Expose component

b3f2da6

Adds styles to backdrop + remove default props.

745907a

Adds transitions to the backdrop and popover.

937272a

Improves popover positioning

8d480bf

Jordi-PV changed the title ~~Adds the popover component~~ Creates the popover component May 8, 2025

Refine and remove console errors

be48a0f

igorschoester reviewed May 8, 2025

View reviewed changes

Jordi-PV mentioned this pull request May 9, 2025

Add in app notifications for classic editor #22223

Merged

18 tasks

Moves the popover into components instead of elements

61ab65d

mhkuu added this to the 25.2 milestone May 12, 2025

mhkuu merged commit 47efeb5 into trunk May 12, 2025
20 checks passed

mhkuu deleted the 22165-creates-a-popover-component-in-the-ui-libray branch May 12, 2025 14:13

Jordi-PV mentioned this pull request May 13, 2025

Adjust the popover component #22260

Closed

Jordi-PV commented May 27, 2025

View reviewed changes

Jordi-PV mentioned this pull request Jun 2, 2025

Adjusts the popover component #22326

Merged

18 tasks

Conversation

Jordi-PV commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Summary

Relevant technical choices:

Test instructions

Test instructions for the acceptance test before the PR gets merged

Relevant test scenarios

Impact check

UI changes

Documentation

Quality assurance

Innovation

Uh oh!

coveralls commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build e10e1cd542012d9b5fa007ab0e7e5d20fcd9487c

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

igorschoester left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jordi-PV Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorschoester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jordi-PV May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jordi-PV May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jordi-PV commented Apr 3, 2025 •

edited

Loading

coveralls commented Apr 3, 2025 •

edited

Loading

Jordi-PV Apr 15, 2025 •

edited

Loading

Jordi-PV May 21, 2025 •

edited

Loading

Jordi-PV May 16, 2025 •

edited

Loading

Jordi-PV left a comment •

edited

Loading