-
Notifications
You must be signed in to change notification settings - Fork 885
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
271 add problem and notification components to alert center #21646
base: trunk
Are you sure you want to change the base?
Conversation
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.
Partial CR 😅
packages/js/src/dashboard/components/hidden-alerts-collapsible.js
Outdated
Show resolved
Hide resolved
packages/js/src/dashboard/components/hidden-alerts-collapsible.js
Outdated
Show resolved
Hide resolved
3ed3932
to
290dfac
Compare
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.
Partial CR 😅
return ( | ||
<div className="yst-flex yst-justify-between yst-gap-2 yst-items-center"> | ||
<Icon className={ classNames( "yst-w-6 yst-h-6", iconClass ) } /> | ||
<Title className="yst-grow" as="h3" size="2">{ title } { counts > 0 && `(${counts})` }</Title> |
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.
- Why
h3
? It seems to me this is semantic level 2 after the Alert center'h1
- Why no count when it is 0?
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.
it says in the design h3, about the counts, I'll ask @ux-tom
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.
A couple more AT remarks!
refactor comment clear property feat: deconstruct context feat: restore styling feat: add safety check for deconstructing the context fix padding in collapsible fix js doc comment fix margin fix collapsable
290dfac
to
476ff2e
Compare
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
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 Add problem card to Alert center