-
Notifications
You must be signed in to change notification settings - Fork 894
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
[UI library] Add toast element #21407
Conversation
Pull Request Test Coverage Report for Build fbadf8a8c26ec0a7ccf4470c8e292df90e9afc0aWarning: 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CR: Approved, just one quirk I don't fully understand 😅 See my comment above.
In my commit above I fixed one issue where the 'Close' story did not have right docs. I also updated some strings and fixed some JSDoc 🙌
@layer components { | ||
.yst-root { | ||
|
||
.yst-toast { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but I would consider placing every CSS rule in a different line for readability. E.g.
.yst-toast {
@apply
yst-w-80
yst-max-w-full
yst-overflow-y-auto
yst-p-4
yst-bg-white
yst-shadow-lg y
st-rounded-lg
yst-ring-1
yst-ring-black
yst-ring-opacity-5
yst-z-20
yst-pointer-events-auto;
}
`.yst-toast--large {
@apply
yst-w-96;
}`
I am also looking at the stylesheets of other elements/components in the UI library, where we also do it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the general approach is to keep the @apply
on the same line as the Tailwind classes when they are a few (like 2 or 3), so I'm updating just the .yst-toast
one 🙂
@@ -0,0 +1 @@ | |||
The `Toast.Close` subcomponent allows to render a button that closes the toast. It is useful when you want to give the user the ability to close the toast manually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I would refactor this doc as follow:
The Toast.Close
subcomponent renders a Toast with a button to close the toast. This feature is useful when you want to provide the user with the option to manually close the toast notification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically the Toast.Close
it is a subcomponent of the Toast
element, so it renders just the close button. I'd rephrase it as:
The Toast.Close subcomponent
renders a button to close the toast. This feature is useful when you want to provide the user with the option to manually close the toast notification.
@@ -0,0 +1 @@ | |||
The Toast component is a general-purpose toast element which can be used to implement custom notifications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I would describe it as:
The Toast
component is a versatile toast element that can be used to implement custom notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counter-proposal 😁 What about:
The Toast
element is a versatile toast component that can be used to implement custom notifications.
In the original description I forgot to change component
to element
, as the Toast
is now an element
@@ -0,0 +1 @@ | |||
`Toast.Description` can be used to add a description to the toast's content. It can be a list of strings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in this doc maybe also mention the paragraph option;
The Toast.Description
subcomponent allows you to add detailed content to the toast, either as a paragraph or a list of strings.
Screen.Recording.2024-05-28.at.14.59.16.movNothing relevant for the AI assessment fixes project, but I was testing the Toast subcomponents
|
Yeah, I should have disabled the |
Context
Toast
element to the UI Library in order to render customizable notifications.Summary
This PR can be summarized in the following changelog entry:
Toast
element.Relevant technical choices:
Notification
component has been refactored to use the newToast
element.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Test the Settings notification
Go to
Yoast SEO
->Settings
Save changes
Go to
Yoast SEO
->Settings
->Site connections
in the
Google
input field type&&&&
and click onSave changes
verify:
Test the AI Generator notification
With a MySQL client open the
wp_usermeta
table and search for the tuple whereuser_id
= your user id andmeta_key
=_yoast_alerts_dismissed
if the following string is present in the
meta_value
field:s:26:"wpseo_premium_ai_generator";b:1;
, remove it and update the array length accordingly (or remove the wholemeta_value
content if you don't care about the notifications you previously dismissed)Activate Yoast SEO Premium
Edit a post in the block editor
Use AI
and verify the following notification appearsDismiss
button closes the notificationUse AI
againTest the UI-Library storybook
yarn storybook
: after some time the StoryBook page should appear in your browserToaster
sub-section in theElements
section and verifyToaster
factory,with title
,with description
andwith close button
) render properlyRelevant test scenarios
Test instructions for QA when the code is in the RC
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes #21396