43 create listing page - basic components#58
Conversation
erikagosti
force-pushed
the
43-create-listing-page
branch
from
43562eb to
4d4cdd2
Compare
naomitzhao
left a comment
naomitzhao
left a comment
There was a problem hiding this comment.
Looks pretty good, great work @erikagosti ! Just left some comments about best practices, mainly styling stuff. Also, please format all of your files using the Prettier extension. See the slide below from Meeting #10:
When you're done addressing all comments, feel free to re-request a review from me or ping me on Slack!
| .page { | ||
| min-height: 100vh; | ||
| background: #f7f7f7; | ||
| padding: 32px 48px 72px; | ||
| color: #1f1f1f; | ||
| margin-top: 100px; | ||
| max-width: 1240px; | ||
| margin-inline: auto; | ||
| } |
There was a problem hiding this comment.
Instead of using margins to define vertical spacing between elements, which can get a bit annoying to keep track of, can we use a flexbox for the page layout? For example, if we were to make the page component a flexbox:
.page {
display: flex;
flex-direction: column;
gap: 80px;
}Note that the gap on Figma is 71px, I'm just using 80px here for cleaner rounded values :P
In general, we should avoid using margin to create layouts if there's a way to achieve it with flexbox instead.
If you combine this with my previous comment about nesting another container inside, these styles should probably be applied on the inner container
You should also be able to get rid of the margin-inline: auto on line 43 as well with these fixes.
There was a problem hiding this comment.
Just bumping this! Unless you have a specific reason not to make the page layout a flexbox, I think it would be best practice to use a flexbox instead of relying on the .actions margin-top to space everything :)
naomitzhao
left a comment
naomitzhao
left a comment
There was a problem hiding this comment.
Yay, looking a lot better! Still a few more things to fix but you're almost there :D
| .page { | ||
| min-height: 100vh; | ||
| background: #f7f7f7; | ||
| padding: 32px 48px 72px; | ||
| color: #1f1f1f; | ||
| margin-top: 100px; | ||
| max-width: 1240px; | ||
| margin-inline: auto; | ||
| } |
There was a problem hiding this comment.
Just bumping this! Unless you have a specific reason not to make the page layout a flexbox, I think it would be best practice to use a flexbox instead of relying on the .actions margin-top to space everything :)
naomitzhao
left a comment
naomitzhao
left a comment
There was a problem hiding this comment.
Just noticed a few spacing things I missed from my previous reviews, my bad 😣 but these should be the last things before I can approve!
2 participants
WIP connect with backend