Updated content link #2102
Merged
Updated content link #2102
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
obulat
added
🟨 priority: medium
obulat
requested review from
fcoveram and
zackkrida
and removed request for
sarayourfriend and
AetherUnbound
|
Size Change: -7.52 kB (-1%) Total Size: 832 kB
ℹ️ View Unchanged
|
obulat
force-pushed
the
add/updated_content_link
branch
from
1263c82 to
cb23096
Compare
AetherUnbound
approved these changes
May 15, 2023
obulat
force-pushed
the
add/updated_content_link
branch
from
cb23096 to
a3da08b
Compare
fcoveram
requested changes
May 16, 2023
There was a problem hiding this comment.
I notice the following visual errors:
- The large variant of component should be
72pxheight - From
smand beyond the component should be large. Here are mockups for all breakpoints. - In small variant, space between media type and results labels should be
4px - In hover and focus, results label changes from "dark charcoal 70" to "dark charcoal"
Regarding the aria-label script, I'm not sure about the "Try an external search source." sentence. The metasearch action is far from the navigation and, in my overall understanding, labels should aim to provide the same information to all users, and this content is not visually conveyed. I might be wrong in this last point
This makes a lot of sense, you're right. I'll remove that part of the sentence. |
obulat
force-pushed
the
add/updated_content_link
branch
2 times, most recently
from
4a9598b to
43b2eae
Compare
obulat
force-pushed
the
add/updated_content_link
branch
2 times, most recently
from
38a03ae to
dfbd2eb
Compare
fcoveram
reviewed
May 18, 2023
obulat
force-pushed
the
add/updated_content_link
branch
from
dfbd2eb to
023430b
Compare
We have removed the top padding (from the |
|
I understand. It makes sense
We can add a top margin of 8px in the search result area. I tried it in Chrome inspector and it looks good. 
I think we should. The paradigm of small views only for small devices is not necessarily true. Having multiple windows in different sizes on the screen is a real use case. Something that Arc browser embraces with a built-in feature. Here is a quick screenshot from a sample browsing.
|
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 #2078 by @fcoveram
Fixes #2083 by @fcoveram
Description
This PR updates the
VContentLinkcomponent to match the styles in the linked issue.Content link is always enabled, even if there are no results (as per #2083).
I also added the
aria-labelto the button to improve the screen reader experience. Currently, it lacks a period between "See all images" and "... results", so it's not easy to understand. The new labels for images are:There are similar labels for audio.
I am a bit worried that having two content links at the top might make screen reader users think that all of the results consist of these two links, and not try to navigate on to the mixed view.
Testing Instructions
Check that the content link on the All results view matches the mockups.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin