Add source, creator and tag links to the single result page media info #3338
Conversation
github-actions
bot
added
🧱 stack: documentation
openverse-bot
added
the
🚦 status: awaiting triage
|
Size Change: +34.9 kB (+4%) Total Size: 1.01 MB
ℹ️ View Unchanged
|
|
Full-stack documentation: https://docs.openverse.org/_preview/3338 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. |
obulat
added
🟨 priority: medium
obulat
force-pushed
the
audio-control/row-box-full
branch
from
8ecdd46 to
28eaa66
Compare
obulat
force-pushed
the
additional_search_views/VByLine
branch
from
646faca to
46122e1
Compare
obulat
force-pushed
the
additional_search_views/VByLine
branch
2 times, most recently
from
0148bd7 to
069e32f
Compare
obulat
changed the title
There was a problem hiding this comment.
Besides the comment shared, I noticed two more things.
- The column and row distance between tag items should be 12px instead of 8px. Here is what I see.
- In
xs, the author and source area don't have the gradient and spacing applied as in the design. Here is what I see. Below you can find the specs for both elements.
Gradient
width: 64px; height: 32px. The white fill goes from 100% to 0% opacity.
Spacing
12px between filter buttons and left and right arrows
| @@ -16,6 +16,21 @@ | |||
| </div> | |||
| </div> | |||
| <div | |||
| v-if="additionalSearchViews" | |||
| class="mx-auto grid grid-cols-1 grid-rows-[auto,1fr] gap-y-6 p-6 pb-0 sm:grid-cols-[1fr,auto] sm:grid-rows-1 sm:gap-x-6 lg:mb-6 lg:max-w-5xl" | |||
There was a problem hiding this comment.
I think here we need to add here the items-center class to align all elements in the middle from sm to beyond. Otherwise they're top aligned, here is what I see.
There was a problem hiding this comment.
After discussing this synchronously, we decided to add a top margin of 4px to the CTA (Get media button) to make it well-balance both for single line titles and longer titles.
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
obulat
force-pushed
the
additional_search_views/VByLine
branch
from
069e32f to
eb757de
Compare
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
obulat
force-pushed
the
additional_search_views/VByLine
branch
from
792afd0 to
9a4c298
Compare
…s/openverse into additional_search_views/VByLine
Signed-off-by: Olga Bulat <[email protected]>
There was a problem hiding this comment.
The spacing changes in sm and beyond are correct, and the scrolling interaction works well.
The only remaining points in xs are:
- Gradient is still not fully white on the sides. As in this screenshot.
- Spacing between left and right action and filter buttons is still tight. As in this screenshot.
- Shaking effect in the pill when the scroll reaches its end. As in this screenshot.
- Left and right buttons don't show up immediately. As in this screenshot.
I can create a new ticket for that as this PR addresses the main changes and the above is more subtle.
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
There was a problem hiding this comment.
I'm not sure if the shaking that Francisco refers to in his review is what I'm seeing in the video below, but I think it presents a major usability issue that looks really easy to run into.
Screencast_20231122_095132.webm
The scenario is basically: I want to scroll all the way to the end of the provider to read the provider name. To do this, I'm repeatedly and quickly clicking the arrow to scroll the view. However, because the arrow disappears at the end and the region I was clicking is now the provider link, I'm suddenly taken to a completely different page. When using normal scrolling arrows, my experience has always been that once you reach the end, the arrow just stops working, rather than disappearing.
The jittering that I think Francisco is also referring to is really jarring if you're trying to read the scrolling text, because the text moves rapidly twice, once to make room for the arrows and then again to actually start scrolling. It'd be one thing if they ended up in the same location, which would at least not cause the jarring effect, but it's ever so slightly different, which means your eyes end up tracking the entire movement and noticing it more.
I don't know how to fix the disappearing arrows usability issue. It feels unavoidable given the design of the arrows appearing. The jittering though, looks like it could be solved by rendering the arrows "above" the scrolling pills, rather than next to them, so that the browser doesn't try to move the contents of the element to make room for the arrows, with the JS update to scroll the contents based on the arrow click coming after the browser paint.
I'm not leaving an approve/request changes on this because I'm not confident about either, so just commenting. If it's not possible to fix these problems in this PR, or if it's better to fix them in a follow up, then I can approve, but on the condition that the feature rollout is blocked on fixing at least one of these issues. The usability problem feels important to me because I think a lot of users will run into it and find it frustrating, but the second is a visual accessibility issue, in my opinion. I think that fixing the first problem would naturally fix the second, which is why I say only one of them needs to be fixed. Really the second one "needs" to be fixed, and if we can fix the first one too it would be a significant improvement to the user experience.
While writing this review and doing more testing, I noticed something I do need to request changes for, however. There is also a big accessibility issue for sighted keyboard users:
In that screenshot the author button is focused. There is a tiny sliver of the purple outline, but otherwise there's no indication that it is focused and, worse, the button is focused but not in view, so I can't even read what it says. Same with the source button. If I tab to the next button, this is what it looks like:
It's almost indistinguishable from the focus state of the previous button and again, I'm unable to read the contents of the button.
My suggestion for fixing this needs @fcoveram's review but is as follows:
- For the focus ring issue, either use a different variant that changes the colour of the button when focused or increase the height of the container so that the top and bottom of the focus ring is visible.
In that screenshot I increased it to 36 px up from 32 and it still doesn't show both the top and the bottom, but it's clearly an improvement. I used taller heights as well but it looks like the buttons need vertical justification within the container as well, because it only adds space to the bottom of the buttons rather than around them. Alternatively, maybe all buttons actually need additional margin to account for the size of the focus ring? When I add margin of 4px to the button I can see the focus ring without making any many changes to the height of the container. That also avoids the vertical justification issue.
2. For the button contents not being visible, automatically scroll the button completely into view when it's focused. I think a non-hijacked scrolling view would do this automatically, so I guess we need to reimplement it?
| const buttonVariant = computed(() => | ||
| uiStore.isBreakpoint("sm") ? "transparent-gray" : "filled-gray" | ||
| ) |
There was a problem hiding this comment.
This approach produces a weird and noticeable lag when resizing a window or rotating a mobile phone.
Is it possible for us to use the ui store only for the initial render and then use breakpoints with display: none (whatever the tailwind equivalent is) to render the correct variant? Something like this pseudo-code:
const serverSide = process.server != undefined
const isSmall = uiStore.isBreakpoint("sm")
const smallVisibilityClass = serverSide && isSmall ? "visible" : "!sm:hidden"
const notSmallVisibilityClass = serverSide && !isSmall ? "visible" : "sm:hidden"Then render the two variants in v-for and apply the visibility class for the variant.
I don't know if that would cause issues on cookieless renders with Nuxt complaining about mismatched client and server renders?
There was a problem hiding this comment.
I used a single button variant, and added tailwind classes with and without sm: prefix to override the colors. Now, the change is much smoother.
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
I also experienced this (and got really annoyed with it) when developing this locally. I assumed that since this is our design, and since Youtube tags also have the same behavior, this is an annoyance that we would have to live with. As a workaround, I added a @sarayourfriend, thank you for flagging the focus ring being cut off. I added a 1.5px padding to the buttons (balanced by -1.5px margins on the container) to stop rings from being cut off. They should now be visible. |
Comparing our implementation with the youtube one, I think the youtube tags don't have as much of a jarring experience to me for two reasons:
A lot of that is definitely improved a lot now though. The only weird thing I'm noticing is that on Maybe we could use real-time calculation of scroll distance? Like, scroll at least 2/3 the width of the container every time, but fudge that a bit so that we're never scrolling less than 1/3? I don't know exactly how to make that math work out, but the idea would be that you'd never have a scroll button press that only scrolled a tiny amount, which is what's happening with the second click of the right scroll for me on that result. Here's a video showing that. Screencast_20231123_121008.webmProbably not blocking, but I haven't fully re-reviewed now. The improvements are awesome though, for sure. I'll re-review tomorrow morning (logging off for the day now). |
|
Great comments here. Three topics have been touched, so grouping them to share my thoughts. Filters interactionThanks @sarayourfriend for sharing your use case. The reason behind using arrows (visible or hidden when reaching the end) comes from an a11y assumption of providing controls, and because some filters' text lengths are quite long and need to be contained within the area. I revisited YouTube again and you're right about the behavior. But I also noticed that buttons are hidden on mobile. It might be because touch interaction predominates in small devices and the scroll relies on swiping, so removing the arrows on small breakpoints could be a good idea. If we can test your suggestion in a different ticket, it would be great to decide which interaction feels smoother. So far, I would merge this PR as it is. Testing the implementation
I'd like to reinforce the idea that design work is not written in stone. If you experience something that feels weird, unexpected, or literally breaks up your experience as a user, please share it. I've been contributing to other WordPress projects and noticed that we don't tend to discuss design ideas and instead develop what's on mockups. We all are Openverse users and our experiences with the tool provide tremendous value. Visible focus
I've mentioned this problem in other tickets and raising the question again of whether is possible to address it for the whole site. If we continue revisiting the focus style per case, we risk missing future use cases like this one, where I completely forgot about it. We can move the discussion into a different ticket, indeed. But to me, this component style belongs to a core state that should not be in revision because of possible layout breaks. |
Thank you for reminding about this, @fcoveram! You are right that we should all be more mindful about the UX when working on the frontend issues.
The core state does have the focus state implemented. However, when an element or its container has a CSS position set, the focus ring gets cut off. It still exists, but it's hidden behind other elements, so it becomes useless. I think it would be nice to open an issue for a better fix for this problem than what I implemented here (adding a padding to the container and removing some of the margins to balance the height with the mockup. |
You are right, I didn't explain myself correctly. It is part of the component.
Mainly to suggest ideas and land on a solution like adding extra space as we do it but from the design stage or another one I'm unaware of. |
|
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @AetherUnbound Excluding weekend1 days, this PR was ready for review 7 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 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
|
I think this was caused by 2 things. The issue has suggested using Scroll.snap.problem.webmI removed the Here's the updated scrolling:
@sarayourfriend, do you think it would make sense to increase the scroll step? I took 150px as "about half of the screen on mobile width", but it might be better to have larger steps? |
I looked at Facebook and Google maps on mobile, and neither of them has arrows for their horizontally scrollable list of tags. I guess this is the behavior users expect, so I would vote for removing the arrows on mobile.
Merging this PR would make it easier to test the PR that adds collection pages, so I would be happy to do as you suggest. @sarayourfriend , do you think this is okay as is, considering that it's under a feature flag and we can open new issues to keep iterating? |
There was a problem hiding this comment.
@obulat Yes, go for it! I intended to leave an approving review on Friday with that exact stipulation. Any of the outstanding issues can be resolved in smaller follow-up PRs, that would be overall easier to review anyway.
I also wanted to say good work on the scrolling box. It is complicated, and I wouldn't have known where to start with it, personally. Despite the small issues, it's a really excellent first pass at it that wouldn't even be bad to release as public, if we were tight on time. The various issues are rather minor and don't overall impede the usability (now that the major a11y focus issue is solved).
For me the issues to resolve are:
- Adjusting the scroll distance to make it feel good (fast enough)
- Potentially removing the arrows on mobile (making sure it's visually clear that there is a scrollable area there, e.g., with the fading)
- Scrolling buttons 100% into view when they're focused via keyboard
Fixes
Fixes #2730 by @obulat
Description
This PR updates the single result page's creator, source and tag links to the new designs, linking them to the collection pages.
Audio
Image
If the creator and source names are longer than the page width, they become scrollable. Setting up the scrolling to work in both LTR and RTL was a bit complex.
Note that the link URLs will be updated after #3140 is merged because it's impossible to add a link to a localized page that does not exist (and we don't currently have pages for
source/creator/tagcollections).Testing Instructions
Run the app
Go to localhost:8443/preferences and switch the
additional_search_viewsflagonGo to single result pages for audio and image.
The source, creator and tag should have an updated style with correct links (
/image/tag/cat,/image/source/flickr,/image/source/flickr/creator/name). Note that i18n (linking to the correct locale if you are not on the English one) does not work yet because the pages for tag/source/creator do not exist yet. The links also do not open the pages because the pages are created in #3140When the creator and the source name are longer than the available width, they become scrollable. To check, see http://localhost:8443/image/8c8f2318-acc8-477c-a059-34f44901933e?q=cat
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin