Closed Brenosalv closed 1 month ago
@travjenkins I'm planning to review all the code before you begin your review. However, if you want to start ahead of me, that's fine too. I created the PR now to ensure we have an initial build version available while I review the code. Also, I'd like to address the ESLint issue soon as they're complicating the review process sometimes decreasing our productivity. Ah, and I'm sorry for the large PR.
@travjenkins Also, just FYI - I created the branch pointing to the max-width branch to start from there. So feel free to skip the commits related to the max-width. If you review by commit, you can start from the commit 6676c19.
Visit the preview URL for this PR (updated for commit 7c3d43f):
https://estuary-marketing--pr295-brenosalv-feature-ne-9tehbp7a.web.app
(expires Thu, 20 Jun 2024 17:51:35 GMT)
🔥 via Firebase Hosting GitHub Action 🌎
Sign: 76f6b095a0752e5d9c6c890267f9fdc3e392161e
I think we need to get a background / "container" to wrap the logos in... otherwise they can easily be totally unreadable.
We don't need to do this right now - but please keep this in me. We have a lot of repeated styling. Using styled
is great - but we do need to try to reduce duplicate styles as much as possible. Instead of styling each child we should really just add styling to a parent that applies to the children OR all the children share a single styled
component to help we the repeated styling
These are what caught my eye
We should probably just make a single reusable wrapper component that has styling for a direct child that applies these styles.
We should probably put some minimum width on the page.
The dots can get a bit weird on an actual mobile device
We should probably put some minimum width on the page.
@travjenkins I was considering the min screen width a device can have is 320px, so I didn't care about widths less than this. But I can add a min-width: 320px
in the Layout component which applies to all the pages. What do you think?
Metrics are looking alright
188
Changes