- Notifications
You must be signed in to change notification settings - Fork927
feat: add margins to pages#1217
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedApr 29, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1217 +/- ##==========================================+ Coverage 65.65% 66.09% +0.43%========================================== Files 269 281 +12 Lines 17366 18400 +1034 Branches 164 216 +52 ==========================================+ Hits 11402 12161 +759- Misses 4772 4975 +203- Partials 1192 1264 +72
Continue to review full report at Codecov.
|
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.
LGTM - comment is minor, looks like a style linter is erroring somewhere.
| export const Margins: React.FC = ({ children }) => { | ||
| const styles = useStyles() | ||
| return ( | ||
| <div> |
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.
Question: It's not clear to me why we need an outer<div> here - do you know what problem this solves?
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.
Yeah, I don't totally understand it, but the content gets horizontally squished without it. My guess is that it's needed as a buffer between the content andStack.
I just would suggest renaming the component to |
@BrunoQuaresma good point but I wanted this to be specific so we can layer different kinds of containers. |
* Add Margin, use constants* Change throughout* Add to a page, lint* Format
This makes our pages look better and should make it easier to keep styles consistent.Fixes#978.
Stack