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
Introduce <Notice>
component for WooCommerce Admin
#44620
Conversation
Hi @lsinger, @mcliwanow, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Test Results SummaryCommit SHA: 7420e78
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Regularising the spelling of isDismissible. Using sanitizeHTML to sanitize the notice description. Making the variant classes like &-success to make them a bit shorter. Adjusting wording of README to convey that this component is designed for the marketplace, though it can be used elsewhere. Tweaking whitespace in one place in the TSX to please the linter.
Good catch @andfinally thanks! |
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.
Looks great, thanks! I just had a few styling suggestions.
The margin between notices should be 32px on mobile.
I'd add a right margin of 12px on .components-button
within a notice to keep buttons clear of each other.
I'd suggest we remove the 3px top margin from .woocommerce-marketplace__notice-icon
and add align-self: flex-start
to .woocommerce-marketplace__notice-close
to make sure these elements align with the top of the text when the content is more than one line.
The close button looks especially odd centred when there is a lot of content.
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.
The component itself looks great 🎉 I left a couple of comments that could improve the code though - things like changing prop types and using CSS class names more consistent with the rest of the codebase.
plugins/woocommerce-admin/client/marketplace/components/notice/README.md
Outdated
Show resolved
Hide resolved
plugins/woocommerce-admin/client/marketplace/components/notice/README.md
Outdated
Show resolved
Hide resolved
plugins/woocommerce-admin/client/marketplace/components/notice/notice.tsx
Outdated
Show resolved
Hide resolved
plugins/woocommerce-admin/client/marketplace/components/notice/notice.tsx
Outdated
Show resolved
Hide resolved
plugins/woocommerce-admin/client/marketplace/components/notice/notice.tsx
Outdated
Show resolved
Hide resolved
plugins/woocommerce-admin/client/marketplace/components/notice/notice.tsx
Outdated
Show resolved
Hide resolved
…/notice.tsx Co-authored-by: Michal Iwanow <4765119+mcliwanow@users.noreply.github.com>
…ored 40px bottom margin to notices on viewports 600px and above. Added height 24px on icons to ensure they're nicely vertically centred.
257221c
to
7420e78
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.
Looks 👌, thanks! I took the liberty of pushing 7420e78 with some small changes, hope that's OK!
- Using the
--{$variant}
pattern for the notices classnames also. - Added
height: 24px;
on the icons to ensure they're nicely vertically centred. - Restored 40px bottom margin to notices on viewports 600px and above.
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR is part of the Sale banner project p9h1Lv-bHt-p2
Closes https://github.com/Automattic/woocommerce.com/issues/19573
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
plugins/woocommerce-admin/client/marketplace/components/content/content.tsx
and import the<Notice>
component withimport Notice from '../notice/notice';
pnpm build
Application > Local Storage
section of your browser console that an entry is saved for that notice. Refresh the page and confirm the hidden notice doesn't show anymore.Changelog entry
Significance
Type
Message
Comment