-
Notifications
You must be signed in to change notification settings - Fork 20
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
Change layout of homepage #3743
Conversation
7165494
to
0441368
Compare
0441368
to
4b7fdc5
Compare
4b7fdc5
to
169e57a
Compare
169e57a
to
6207687
Compare
6207687
to
573bcf6
Compare
573bcf6
to
ab3f8f1
Compare
ab3f8f1
to
21d89da
Compare
21d89da
to
ff075b5
Compare
ff075b5
to
92f7dc3
Compare
fa35c9c
to
79d40c9
Compare
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.
Some minor comments on the CSS. Still need to review the markup changes.
57526f0
to
098aafc
Compare
The CSS is used for the grid is taken from the `cards` component.
098aafc
to
42ccdc7
Compare
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.
Great, thanks both @MartinJJones and @patrickpatrickpatrick for getting this into such a good place 🙌 We should probably now update the PR description as it's now longer reflective of the work, there have been quite a few design changes since.
I tested the page on the browsers and devices we usually test in. Didn't spot any issues, apart from how on mobile there seems to be a bit too much grey space below the Popular links. I checked with Monica who agreed that we should reduce the amount of space so that's something to look at tomorrow.
I'm giving this PR a tick to indicate that I'm happy with everything apart from the above mobile spacing issue so whoever reviews the PR tomorrow can focus on reviewing any small fixes we add tomorrow.
a7477d4
to
116017c
Compare
@hannalaakso thanks for finding and reporting the spacing issue on mobile, I've fixed this issue on a recent commit: |
- Ensures that the total padding-bottom on desktop underneath Popular links is 50px (30px + 20px from the popular links) - Removed grid-auto-rows: fraction(1) as it is not required
116017c
to
ba43a98
Compare
ba43a98
to
d0dd87f
Compare
553599a
to
1feee47
Compare
Temp style overrides to the image card component from the gem to: - Ensure the space between the image card and the text remains at 15px on mobile, this was previously 30px on mobile. - Ensure the featured image cards have 15px padding to the left and right when this screen width is =< 319px.
1feee47
to
4363b40
Compare
I've also completed a secondary review of changes made today and it looks good to go. |
What
Change the layout of homepage so that:
Why
As part of several incremental changes being made to the homepage.
Trello Card
Visual Changes
Before
After