Creates the popover component #22170
Conversation
Pull Request Test Coverage Report for Build ca99786b69a25106723450181d6049c34982ff80Warning: 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.
Details
💛 - Coveralls |
Jordi-PV
added
the
changelog: non-user-facing
There was a problem hiding this comment.
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
| <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" |
There was a problem hiding this comment.
Some line stuff in Chrome:
There was a problem hiding this comment.
Now the spacing is gone? So that is a UX question?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| leaveFrom="yst-bg-opacity-75" | ||
| leaveTo="yst-bg-opacity-0" | ||
| > | ||
| <div className={ classNames( "yst-popover-backdrop", className ) } /> |
There was a problem hiding this comment.
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"
/>| useEffect( () => { | ||
| if ( isVisible ) { | ||
| document.body.classList.add( "backdrop-active" ); | ||
| } else { | ||
| document.body.classList.remove( "backdrop-active" ); | ||
| } | ||
| }, [ isVisible ] ); |
There was a problem hiding this comment.
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
| isVisible, | ||
| setIsVisible, | ||
| position, | ||
| backdrop, |
There was a problem hiding this comment.
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?
| Popover.Title = Title; | ||
| Popover.CloseButton = CloseButton; | ||
| Popover.Content = Content; | ||
| Popover.Backdrop = Backdrop; |
There was a problem hiding this comment.
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";
| ); | ||
| } ); | ||
|
|
||
| Popover.displayName = "Popover"; |
| } | ||
| } | ||
|
|
||
| .yst-popover--top { |
There was a problem hiding this comment.
The very fine line that divides the arrow and the tooltip itself should probably not be there?
| <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" |
There was a problem hiding this comment.
Now the spacing is gone? So that is a UX question?
| yst-flex | ||
| yst-self-start; | ||
|
|
||
| & button { |
| export default { | ||
| title: "1) Elements/Popover", | ||
| component: Popover, | ||
| argTypes: { |
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
.yst-rootvariables from the @yoast/components monorepo stylesheet dependency and displayed in Storybook.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
yarn workspace @yoast/ui-library storybookto open the storybook on your local server.Relevant test scenarios
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Documentation
Quality assurance
Innovation
innovationlabel.Fixes #22165