Update the store permission error notices to match the new design.#13115
Open
JakePT wants to merge 4 commits into
Open
Update the store permission error notices to match the new design.#13115JakePT wants to merge 4 commits into
JakePT wants to merge 4 commits into
Conversation
🎭 Playwright reports for 71c9489: 📚 Storybook for 71c9489: 📦 Build files for 71c9489:
|
JakePT
marked this pull request as ready for review
July 15, 2026 03:37
|
Size Change: 0 B Total Size: 3.71 MB ℹ️ View Unchanged
|
ankitrox
requested changes
Jul 22, 2026
Comment on lines
+98
to
+113
| userInfo = setupFlowRefreshPhase4Enabled | ||
| ? sprintf( | ||
| /* translators: %s: owner name */ | ||
| __( | ||
| 'This service was originally connected by the administrator "%s", you can contact them for more information.', | ||
| 'google-site-kit' | ||
| ), | ||
| owner.login | ||
| ) | ||
| : sprintf( | ||
| /* translators: %s: owner name */ | ||
| __( | ||
| 'This service was originally connected by the administrator "%s" — you can contact them for more information.', | ||
| 'google-site-kit' | ||
| ), | ||
| owner.login |
Collaborator
There was a problem hiding this comment.
These changes will also affect the ReportError component when setupFlowRefreshPhase4 feature flag is enabled which is not in scope of this issue.
Collaborator
Author
There was a problem hiding this comment.
@techanvil What do you think about this? Seems silly to add an exception so that ReportError uses the old copy even with setupFlowRefreshPhase4. Once that feature's live we'd have to keep both versions around.
Collaborator
There was a problem hiding this comment.
Collaborator
There was a problem hiding this comment.
Add the tests in ErrorNotice.test.js for the new title, prefix suppression, and retry-message suppression behavior. Test coverage section mentioned about the same, but PR does not contain these tests.
Collaborator
Author
There was a problem hiding this comment.
@ankitrox Do we really need whole unit tests just for basic boolean attributes? The new story on assets/js/components/ErrorNotice.stories.tsx and the VRT already verifies that these attributes work as expected in controlling the output.
Collaborator
There was a problem hiding this comment.
@JakePT as the behaviour is effectively covered by the VRT, it's fair to say we've got test coverage and don't need the JS tests.
You've already mentioned this in "relevant technical choices", which is good to see.
Collaborator
Author
|
@ankitrox As per Tom's comments no changes are required for those items, but I've updated the QAB to verify the |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Addresses issue:
Relevant technical choices
The IB called for new tests in
assets/js/components/ErrorNotice.test.jsbut I couldn't identify any new functionality worth testing. The only change to this component is support for thetitleprop which is adequately tested by the new story inassets/js/components/ErrorNotice.stories.js. The logic around the feature flag and which design to use is contained inassets/js/components/StoreErrorNotices.js, so that's covered by the new tests inassets/js/components/StoreErrorNotices.test.js.PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist