Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(react,styles): add Notice component #1136

Merged
merged 25 commits into from
Aug 2, 2023
Merged

feat(react,styles): add Notice component #1136

merged 25 commits into from
Aug 2, 2023

Conversation

tbusillo
Copy link
Contributor

@tbusillo tbusillo commented Jul 31, 2023 •
edited
Loading

closes: #1031

@github-actions
Copy link
Contributor

Preview branch generated at https://add-notice-component.d1gko6en628vir.amplifyapp.com

@tbusillo tbusillo marked this pull request as ready for review July 31, 2023 18:22
@tbusillo tbusillo requested review from a team as code owners July 31, 2023 18:22
scurker
scurker requested changes Aug 1, 2023
View reviewed changes
Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far! Just some comments/thoughts after reviewing below:

packages/styles/notice.css Outdated Show resolved Hide resolved
packages/styles/notice.css Outdated Show resolved Hide resolved
packages/styles/notice.css Outdated Show resolved Hide resolved
packages/styles/notice.css Outdated Show resolved Hide resolved
packages/react/src/components/Notice/index.tsx Outdated Show resolved Hide resolved
docs/pages/components/Notice.mdx Outdated Show resolved Hide resolved
docs/pages/components/Notice.mdx Outdated Show resolved Hide resolved
docs/pages/components/Notice.mdx Outdated Show resolved Hide resolved
docs/pages/components/Notice.mdx Outdated Show resolved Hide resolved
docs/pages/components/Notice.mdx Outdated Show resolved Hide resolved
scurker
scurker reviewed Aug 1, 2023
View reviewed changes
docs/pages/components/Notice.mdx Outdated Show resolved Hide resolved
docs/pages/components/Notice.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Notice/index.tsx Outdated Show resolved Hide resolved
@tbusillo tbusillo requested a review from scurker August 2, 2023 14:41
scurker
scurker previously approved these changes Aug 2, 2023
View reviewed changes
Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shawnsharpDQ
Copy link

I am noticing one discrepancy, @tbusillo looks like the top and bottom padding should be 12px not 16px

@tbusillo tbusillo requested a review from scurker August 2, 2023 14:58
@tbusillo
Copy link
Contributor Author

tbusillo commented Aug 2, 2023

I am noticing one discrepancy, @tbusillo looks like the top and bottom padding should be 12px not 16px

@shawnsharpDQ good catch! Updated it and @scurker, I updated the test/prop types that were throwing an error.

scurker
scurker previously approved these changes Aug 2, 2023
View reviewed changes
@scurker scurker dismissed their stale review August 2, 2023 15:04

changes needed

scurker
scurker requested changes Aug 2, 2023
View reviewed changes
Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last change and then we should be good to go!

docs/pages/components/Notice.mdx Outdated Show resolved Hide resolved
@tbusillo tbusillo requested a review from scurker August 2, 2023 19:03
@tbusillo tbusillo enabled auto-merge (squash) August 2, 2023 19:09
@scurker scurker mentioned this pull request Aug 2, 2023
Closed
@tbusillo tbusillo merged commit 14bfe32 into develop Aug 2, 2023
2 checks passed
@tbusillo tbusillo deleted the add-notice-component branch August 2, 2023 19:21
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

Preview branch generated at https://add-notice-component.d1gko6en628vir.amplifyapp.com

@github-actions github-actions bot mentioned this pull request Aug 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scurker scurker scurker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Notice Component
3 participants
@tbusillo @shawnsharpDQ @scurker