-
Notifications
You must be signed in to change notification settings - Fork 38
feat(Layout): Aside, Footer & Header now support border* props #2016
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2016 +/- ##
==========================================
+ Coverage 93.59% 93.69% +0.09%
==========================================
Files 404 405 +1
Lines 6702 6739 +37
Branches 2152 2168 +16
==========================================
+ Hits 6273 6314 +41
+ Misses 429 425 -4
Continue to review full report at Codecov.
|
ghost
left a comment
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.
Can you add Storybook stories for Header, Aside & Footer so that snapshots are added for this feature (as well as upcoming ones)
packages/components/src/Layout/Semantics/semanticBorderHelper.ts
Outdated
Show resolved
Hide resolved
6323476 to
75b05b9
Compare
ghost
left a comment
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.
Functionality looks great!
Had some comments about Storybook & test coverage adjustments I'd like to see.
packages/components/src/Layout/Semantics/Story/ExamplePage.story.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/Layout/Semantics/Story/Semantics.story.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/Layout/Semantics/semanticBorderHelper.test.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/Layout/Semantics/semanticBorderHelper.test.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/Layout/Semantics/semanticBorderHelper.ts
Outdated
Show resolved
Hide resolved
33d79f9 to
9729dd3
Compare
adcddef to
5cdf15d
Compare
5cdf15d to
513e4f0
Compare
ghost
left a comment
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.
Two really tiny nits on test-coverage / stories and then this should be good-to-go.
packages/components/src/Layout/Semantics/Story/Semantics.story.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/Layout/Semantics/semanticBorderHelper.test.tsx
Outdated
Show resolved
Hide resolved
b5fc877 to
e195fef
Compare
e195fef to
005a777
Compare
ghost
left a comment
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! Merge away! :)
ghost
left a comment
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! Merge away!
Requirements
Please check the following items are addressed in your pull request (or are not applicable)
Image snapshots (choose one)
Documentation updated (choose one)