[@wordpress/notices] Add removeAllNotices action to allow all notices to be removed from a given context
#44059
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
opr
added
[Type] Performance
opr
requested review from
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: +32 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
talldan
reviewed
Sep 12, 2022
| * | ||
| * @return {Object} Action object. | ||
| */ | ||
| export function removeAllNotices( context = DEFAULT_CONTEXT ) { |
There was a problem hiding this comment.
One thing about this API is calling it would remove both 'snackbar' notices and also notices that have a type of 'default'.
From a UI point of view, it might look unusual to see two things that are visually different suddenly disappear from a single dismissal, so I wonder if there should be a way to filter by type as well. The same feedback possibly also applies to #39940, though probably less so, as snackbar notices are likely to have a different id.
What do you think?
Something that would be helpful is an example of the use case this PR is fulfilling. #39940 shows the developer requirement, but doesn't describe the feature this API supports from a user perspective (if there is one).
There was a problem hiding this comment.
@talldan thanks for the review - I agree that allowing filtering by type is a great idea. I will implement this in both PRs.
The difference between this and #39940 is this one does not require any IDs to be passed to the action which will confer a slight performance improvement based on selecting fewer values from the store and dispatching fewer actions.
A use-case I had in mind for this would be if an app is showing multiple notices, and there is the option to clear all notices, then currently they'd need to loop through the notices and get each ID, then remove each individually. Admittedly the user experience is no different if all notices are removed in one action, or n actions, the end-result is the same.
There was a problem hiding this comment.
I'm not convinced that filtering by type in #39940 is a good idea - the developer would have to already know the IDs to remove them, so asking them to pass the type as well seems like it would add more friction for not much gain. I know a snackbar notice and default notice can both have the same ID, but removeNotice does not ask for a type, so when using this it would remove notices of both type anyway.
opr
force-pushed
the
add/remove-all-notices
branch
from
effe1ce
to
4e65fdc
Compare
opr
force-pushed
the
add/remove-all-notices
branch
from
d79dbd6
to
207db72
Compare
|
Hey @talldan I was wondering if you could take another look at this, or if you have any further thoughts about adding this action? Happy to hear feedback and implement any suggestions you might have. Also just noting a real-world use case for this in https://github.com/woocommerce/woocommerce-blocks/blob/bd69c1fabe5f1734f4821c197e9ebda1bd0e0c79/assets/js/base/utils/create-notice.ts#L88 |
Merged
1 task
10 tasks
opr
force-pushed
the
add/remove-all-notices
branch
from
207db72
to
06f82a8
Compare
|
Flaky tests detected in e879e72f41b5a63b9f8aa906f1c367cbf678500b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5122396045
|
|
@nerrad this has been rebased and tested. On my end it works OK. |
nerrad
approved these changes
May 30, 2023
There was a problem hiding this comment.
LGTM, pre-approving but before this is merged it needs a changelog entry. Once you added that I can get it merged.
opr
force-pushed
the
add/remove-all-notices
branch
from
e879e72
to
e6cb885
Compare
nerrad
reviewed
May 30, 2023
Comment on lines
+30
to
+31
| case 'REMOVE_ALL_NOTICES': | ||
| return state.filter( ( { type } ) => type !== action.noticeType ); |
There was a problem hiding this comment.
This isn't considering the provided context in the action object. Should it?
There was a problem hiding this comment.
This isn't considering the provided context in the action object. Should it?
The onSubKey in the reducer takes care of that actually.
I have tests that cover removing notices from a specific context: https://github.com/wordpress/gutenberg/blob/2a5db119b8ed8e16002de157a5edf37ff47dffe6/packages/notices/src/store/test/reducer.js#L230
nerrad
approved these changes
May 30, 2023
nerrad
approved these changes
May 30, 2023
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…ices to be removed from a given context (WordPress#44059) * Add removeAllNotices action with example and docs * Add actions tests for removeAllNotices * Add reducer case to handle REMOVE_ALL_NOTICES action type * Add tests for REMOVE_ALL_NOTICES action type * Add code fence around action example * Add noticeType to action args and tests * Change reducer and tests to account for notice type * Add changelog entry * Fix lint error
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Add a
removeAllNoticesaction to allow all notices to be removed from a context without requiring knowledge of each of their IDs.Why?
When clearing notices you currently need to know each notice's ID and remove it individually resulting in several different actions being dispatched to the store.
#39940 proposes the addition of
removeNoticeswhere the IDs of multiple notices to be removed can be specified, however this still requires the caller to know the IDs of all notices.This action further streamlines the use-case of removing all notices from a context by not requiring any IDs, and instead just emptying the context entirely.
How?
The reducer will return an empty array when this action is dispatched.
Testing Instructions
npm run testand check all unit tests pass.Test 1error.Screenshots or screencast