Update logo nav component in header #2136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +3.53 kB (0%) Total Size: 850 kB
ℹ️ View Unchanged
|
dhruvkb
reviewed
May 19, 2023
obulat
force-pushed
the
fix/header_home_link
branch
from
5c966c1 to
a7982c5
Compare
obulat
added
✨ goal: improvement
That was a locally-generated snapshot that I've updated later. I think we don't need to pause animation, but if the tests do fail again, I'll investigate if we need to adjust the wait. I've replaced the snapshot with a correct one. |
obulat
force-pushed
the
fix/header_home_link
branch
2 times, most recently
from
e87d12f to
ca6191e
Compare
obulat
force-pushed
the
fix/header_home_link
branch
2 times, most recently
from
5c774f5 to
5a0ec34
Compare
|
This looks excellent. Before approving I do want to clarify one thing with @fcoveram when he can look at it: There's historically been, including in this PR, a small size and placement difference between the logo on the homepage and the logo on the site content pages. I attempted to make a video overlaying the two to show the issue: https://share.cleanshot.com/TSbFf3kz is this discrepancy present in Figma? Does it matter to fix this? |
obulat
force-pushed
the
fix/header_home_link
branch
from
5a0ec34 to
375c672
Compare
9 tasks
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @fcoveram Excluding weekend1 days, this PR was ready for review 2 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
AetherUnbound
approved these changes
May 24, 2023
As in the example shared, it's not present in Figma. If you go to the Logo nav component in the Design Library, you will see the following 
Row 1 (size: small) is the logo for the content header in small breakpoints when it's part of the search bar. Row 2 (size: medium) is for the content header in bigger breakpoints. And Row 3 (size: large) is for the internal header in all breakpoints. Row 2 and 3 share the same size for the symbol, but the latter adds the word "openverse" and has a 16px padding on left and right sides. 
So going back to your questions from the video:
|
obulat
force-pushed
the
fix/header_home_link
branch
from
375c672 to
d91df61
Compare
fcoveram
approved these changes
May 24, 2023
There was a problem hiding this comment.
It looks perfect ✨ 🚀
Since this PR addresses changes in Header internal, the component specs described here can be tackled in a different PR.
obulat
force-pushed
the
fix/header_home_link
branch
from
84bae3f to
214ab38
Compare
obulat
force-pushed
the
fix/header_home_link
branch
from
ae94e43 to
b02047b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Related to #476 by @fcoveram
Description
This PR updates the
VHomeLinkcomponent and theVLogoButtoncomponent to match the Figma mockups. It cleans up the components from unused props and adds the Storybook stories.This PR is necessary for replacing the svg dependencies in #2135, which, in turn, is required for Nuxt 3 migration. This is the reason for "high" priority label.
Because this PR changes the spacing of the header logo, there are a lot of snapshot changes.
Testing Instructions
Check the home links in the internal header (the one with the page links, as on the homepage, content pages such as "about" page and the single result pages). The logo should be 18px high, as in the mockups, and the link itself should be 48px high. On hover on white pages, the background becomes yellow. When focused, it should have a pink focus ring.
A special case is a pages modal on mobile width screen. You can open it if you open the site on a narrow screen and click on the "hamburger" menu button. The modal background is black, so the logo becomes white. The focus ring is yellow instead of pink.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin