Update footer to match new mockups #2015
Conversation
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/2015 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
openverse-bot
added
the
🚦 status: awaiting triage
|
@obulat should there be accompanying updates to visual regression tests here? Thanks for the speedy implementation! |
|
Size Change: -2.02 kB (0%) Total Size: 867 kB
ℹ️ View Unchanged
|
obulat
added
🟧 priority: high
There was a problem hiding this comment.
It looks great, I just noticed the following details.
Content version
Spacings
- In
xland2xlthe left and right padding should be40pxinstead of24px - In
lg,xl, and2xl, the gap between rows should be32pxinstead of40px
Components
For the page links, I think you used the "Clear" component from the drawer. In theory, that is correct, but the element has a fixed height to match the spacing rule of 8x.
The element has a decimal size of 32.9, which can be problematic and break layouts if we keep using it as it is. We can tackle this in two ways I think:
- Replace the Clear component with the Link component and apply the spacing rule to the gap between
lielements - Set a fixed height of the Clear component as
32pxand add a new spacing value betweenlito match the mockup.
I am drawn to the first option as the Link component was thought for links single links, whereas the Clear component inherits the fixed height from the Results button. Why did this happen? Because the link component did not exist at the moment I started the header redesign.
In sm, the locale component has a max-width: 100% that crashes the layout. The mockup keeps the width fixed as 230px in sm, md, lg, xl, 2xl to avoid this effect. Is that possible or better to find a different solution?
Alignment
In sm and md, the locale component and WordPress call need to be aligned vertically centered.
Internal version
Spacings
- In
xland2xlthe left and right padding should be40pxinstead of24px - In
smandmd, padding values should be24px, 24px, 40px, 24px. The top part is bigger to match the result area layout.
Alignment
- In
smandmd, the WordPress call should be fixed on the right side. - In
smandmd, the locale component and WordPress call need to be aligned vertically centered.
obulat
force-pushed
the
footer_adjustments
branch
3 times, most recently
from
9466d23 to
c234cec
Compare
obulat
force-pushed
the
footer_adjustments
branch
from
cd9253f to
eef955a
Compare
|
@obulat when this is ready for re-review could you ping reviewers / re-request reviews? |
There was a problem hiding this comment.
It looks great. Just a few notes
Content version
- Top and bottom paddings must be
40pxin all breakpoints.
Internal version
- In
lg, xl, 2xlall sides should be40px. I see a24pxfor the bottom side. - In
sm, mdthe top and bottom spacings are inverted. It should be40pxfor the bottom and24pxfor the top.
Nav items
Despite the above changes. Which one do you think is the best option to address the nav items' components?
obulat
force-pushed
the
footer_adjustments
branch
from
b0acb39 to
2fe3755
Compare
|
Thank you for the review, @panchovm! I've fixed the footer paddings.
What kind of problems will the decimal size of 32.9 bring? I don't think we can set a fixed height in the navigation bar because we need the designs to be used in other languages, too. Some navigation items can have a long title when translated, and then they will not fit into the component and will overflow. So, we have to set the height using text height + padding. The text height, when it's in one line, is decimal, 16.5px. We either need to set a decimal padding to get a nice link height of 32px, or we have to live with a decimal height of the link: In the code, we don't really have a This is what the height becomes for one-line items: And here, what items with more than one-line of text will look like: Is this approach OK, or should we think of something else, @panchovm? |
obulat
force-pushed
the
footer_adjustments
branch
from
171ccc5 to
dbcc334
Compare
obulat
force-pushed
the
footer_adjustments
branch
from
d64d4c0 to
bba5491
Compare
Today I learned: it's ok for the tappable area (or the target size on the desktop) to be smaller than 44px if the areas do not overlap. So, it's OK from the accessibility point of view (or SEO in lighthouse checks) if the focus area of the links is small, like in the snapshots you shared, @panchovm, but there is enough space between the links. The problem starts when the links are so close to each other that a click on one can be interpreted as a click on another one. And we don't have such problem anyways :) I've updated the code to have a small focus "box" around the links. The padding now is on the |
obulat
force-pushed
the
footer_adjustments
branch
from
7a4b229 to
5e386fe
Compare
|
@obulat this looks fantastic visually and I'll get you a more in-depth review about the code itself when I have a chance to review it later today. Thanks! |
Extract VWordPressLink; use VPageLinks for footer nav
obulat
force-pushed
the
footer_adjustments
branch
from
83256d7 to
ec527ff
Compare
| > | ||
| <template #wordpress> | ||
| <WordPress class="aria-hidden" /> | ||
| <span class="sr-only">WordPress</span> |
Fixes
Fixes #2012 by @obulat
Related to #2009
Description
This PR updates the footer to match the latest mockups.
The footer layout should be updated to match the screenshot.
Mockup
Testing Instructions
The footer should work properly, and match the mockups.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin