SnackbarList, Snackbar: Add unit test #59157

t-hamano · 2024-02-19T01:26:16Z

Preparation to tackle #59139

What?

This PR adds unit tests for SnackbarList and Snackbar components.

Why?

While investigating #59139, I discovered that there are no unit tests for the Snackbar component.

Testing is necessary to ensure that the current implementation works as specified or to prevent future regressions.

How?

We tested as many cases as possible according to the actual implementation and README.

Testing Instructions

Run the following command:

npm run test:unit packages/components/src/snackbar/test/index.tsx
npm run test:unit packages/components/src/snackbar/test/list.tsx

test

t-hamano · 2024-02-19T01:31:58Z

packages/components/src/snackbar/index.tsx

-			role={ ! explicitDismiss ? 'button' : '' }
+			role={ ! explicitDismiss ? 'button' : undefined }
 			onKeyPress={ ! explicitDismiss ? dismissMe : undefined }
-			aria-label={ ! explicitDismiss ? __( 'Dismiss this notice' ) : '' }
+			aria-label={
+				! explicitDismiss ? __( 'Dismiss this notice' ) : undefined
+			}


Prevents attributes without values from being applied.

The negation in all of these ternaries feels totally backwards to me, but hey ho... at least they're consistent!

Perhaps instead we could separate out the dismiss props?

// `explicitDismiss || { ... }` also works, // as long as it's guaranteed to be a boolean! const dismissProps = explicitDismiss ? {} : { onClick: dismissMe, onKeyPress: dismissMe, role: 'button', 'aria-label': __( 'Dismiss this notice' ), } return ( <div { ...dismissProps } /> );

Anyway, scope creep 🚨

Although this is a negative form, how about the following approach?

const dismissProps = { onClick: dismissMe, onKeyPress: dismissMe, role: 'button', 'aria-label': __( 'Dismiss this notice' ), }; return ( <div // When there is no explicit close button to close the snackbar, // give the snackbar itself a prop that can be dismissed. { ...( ! explicitDismiss && dismissProps ) } > );

t-hamano · 2024-02-19T01:33:14Z

packages/components/src/snackbar/stories/list.story.tsx

@@ -70,7 +70,6 @@ Default.args = {
 				},
 			],
 			content: 'Post published.',
-			isDismissible: true,


isDismissible is not present in the Snackbar component's action property.

Can you clarify what you mean here? isDismissible is a valid NoticeProp, which SnackbarListProps.notices ultimately derives from.

Oh wait... this bit?

It's a little confusing, but the Snackbar component inherits the type of the Notice component. As mentioned in this comment, the current Snackbar component inherits a type that contains unintended properties.

In fact, if you search the ./packages/components/src/snackbar directory with isDismissible, you shouldn't find any code containing isDismissible.

t-hamano · 2024-02-19T01:37:12Z

packages/components/src/snackbar/test/index.tsx

+		} );
+	} );
+
+	describe( 'useSpokenMessage', () => {


This is the same as testing the Notice component. We are testing the useSpokenMessage hook itself, and if it has been tested with the Notice component, there might be no need to test it again here.

t-hamano · 2024-02-19T01:40:21Z

packages/components/src/snackbar/test/list.tsx

+ */
+import SnackbarList from '../list';
+
+window.scrollTo = jest.fn();


When the Snackbar component is removed, the animation runs through motion.div. If we don't mock window.scrollTo, the following error seems to occur.

expect(jest.fn()).not.toHaveErrored(expected) Expected mock function not to be called but it was called with: [[Error: Not implemented: window.scrollTo]],[[Error: Not implemented: window.scrollTo]]

github-actions · 2024-02-19T01:47:04Z

Flaky tests detected in 65f7b6b.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7953430355
📝 Reported issues:

[Flaky Test] default to a list of pages if there are no menus #47732 in /test/e2e/specs/editor/blocks/navigation.spec.js
[Flaky Test] default to the only existing classic menu if there are no block menus #48033 in /test/e2e/specs/editor/blocks/navigation.spec.js

t-hamano · 2024-02-19T01:55:01Z

packages/components/src/snackbar/types.ts

@@ -28,8 +32,32 @@ type SnackbarOnlyProps = {
 	listRef?: MutableRefObject< HTMLDivElement | null >;
 };

-export type SnackbarProps = Omit< NoticeProps, '__unstableHTML' > &
-	SnackbarOnlyProps;
+export type SnackbarProps = Pick<


This was the part I had the most trouble with. This component uses the types of the Notice component, and I discovered that the following types, which are not available in the Snackbar component, also apply to the Snackbar component.

isDismissible

status

actions.className

actions.noDefaultClasses

actions.variant

I used Pick instead of Omit so that when new props are added to the Notice component in the future, they are not unintentionally exposed to the Snackbar component as well.

At the same time, the code editor should also correctly display the description of the actions prop.

github-actions · 2024-02-19T01:56:01Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: andrewhayward <andrewhayward@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

tyxla · 2024-02-26T12:49:15Z

packages/components/src/snackbar/index.tsx

@@ -138,9 +138,12 @@ function UnforwardedSnackbar(
 			className={ classes }
 			onClick={ ! explicitDismiss ? dismissMe : undefined }
 			tabIndex={ 0 }
-			role={ ! explicitDismiss ? 'button' : '' }
+			role={ ! explicitDismiss ? 'button' : undefined }


May be a nitpick, and I might be missing something, but shouldn't an element without a role have a null role instead of an undefined one?

Also, is "no-role" the best to do in this case? Is alert or any other status role a better fit perhaps? @andrewhayward what do you think?

packages/components/src/snackbar/test/index.tsx

packages/components/src/snackbar/test/list.tsx

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

tyxla

Nothing further from my perspective. Nice work 🚀

t-hamano · 2024-02-28T02:04:02Z

Thanks for the review!

This PR also includes a change to remove the role attribute which has no value, which I believe is at least an improvement and not a regression. I would like to merge this PR as I think that what role is appropriate or not if the Snackbar is not a button, or any refactoring mentioned in this comment could also be addressed in a follow-up.

SnackbarList, Snackbar: Add unit test

d00d22b

test

t-hamano added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Feb 19, 2024

t-hamano self-assigned this Feb 19, 2024

Update changelog

e93b1f7

t-hamano commented Feb 19, 2024

View reviewed changes

Use testId variable

65f7b6b

t-hamano commented Feb 19, 2024

View reviewed changes

t-hamano requested a review from a team February 19, 2024 01:55

t-hamano marked this pull request as ready for review February 19, 2024 01:55

t-hamano requested a review from ajitbohra as a code owner February 19, 2024 01:55

t-hamano mentioned this pull request Feb 19, 2024

"View XXX" displayed in the snack bar should open in a new tab #59139

Open

tyxla reviewed Feb 26, 2024

View reviewed changes

t-hamano and others added 7 commits February 27, 2024 12:58

Use single element query

8d52908

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

Use toBeVisible instead of toBeInTheDocument

f22ae17

Remove unecessary resetAllMocks()

bcfb769

Make the query more precise

0d5e29f

Use toHaveBeenCalledTimes(1) instead of toHaveBeenCalled()

326b854

Merge branch 'trunk' into snackbar/add-unit-test

6ccef42

Move changelog entry into Unreleased section

ee2df5a

tyxla approved these changes Feb 27, 2024

View reviewed changes

t-hamano merged commit 33f5094 into trunk Feb 28, 2024
60 of 61 checks passed

t-hamano deleted the snackbar/add-unit-test branch February 28, 2024 02:08

github-actions bot added this to the Gutenberg 17.9 milestone Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SnackbarList, Snackbar: Add unit test #59157

SnackbarList, Snackbar: Add unit test #59157

t-hamano commented Feb 19, 2024 •

edited

t-hamano Feb 19, 2024

andrewhayward Feb 20, 2024

t-hamano Feb 21, 2024

t-hamano Feb 19, 2024

andrewhayward Feb 20, 2024

andrewhayward Feb 20, 2024

t-hamano Feb 21, 2024

t-hamano Feb 19, 2024

t-hamano Feb 19, 2024

github-actions bot commented Feb 19, 2024 •

edited

t-hamano Feb 19, 2024

github-actions bot commented Feb 19, 2024 •

edited

tyxla Feb 26, 2024

tyxla Feb 26, 2024

tyxla left a comment

t-hamano commented Feb 28, 2024

SnackbarList, Snackbar: Add unit test #59157

SnackbarList, Snackbar: Add unit test #59157

Conversation

t-hamano commented Feb 19, 2024 • edited

What?

Why?

How?

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 19, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Feb 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

t-hamano commented Feb 28, 2024

t-hamano commented Feb 19, 2024 •

edited

github-actions bot commented Feb 19, 2024 •

edited

github-actions bot commented Feb 19, 2024 •

edited