Fix audio snackbar and box audio seeking #2743
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
openverse-bot
added
🟧 priority: high
|
Size Change: -3.35 kB (0%) Total Size: 868 kB
ℹ️ View Unchanged
|
obulat
force-pushed
the
fix/snackbar_show_on_mouse_focus
branch
from
8556807 to
a8f3747
Compare
obulat
force-pushed
the
fix/snackbar_show_on_mouse_focus
branch
from
a8f3747 to
03415c9
Compare
sarayourfriend
approved these changes
Aug 7, 2023
Comment on lines
146
to
154
| /** | ||
| * Whether instructions snackbar should be shown when this track is focused. | ||
| * This is used to show instructions for keyboard navigation on the search | ||
| * results page. | ||
| */ | ||
| showsSnackbar: { | ||
| type: Boolean, | ||
| default: false, | ||
| }, |
There was a problem hiding this comment.
When is this meant to be false? In other words, in what case do we not show the instructions when keyboard navigating?
There was a problem hiding this comment.
I've asked @fcoveram about it, and we decided that we want to show the instructions for all audio tracks (even the main audio track for the single result page that didn't use to show it), and to hide it when the audio track loses focus.
I updated the PR to reflect that. So, now when you focus on any audio track, you see the instructions snackbar. It is completely dismissed if you interact with the audio using your keyboard. If you move the focus away from the audio track, the snackbar is only hidden (so, it re-appears if you focus on an audio track again).
There was a problem hiding this comment.
How does mobile factor in here? I just noticed on the live site that clicking the boxed audio layout shows the snack bar on mobile
obulat
force-pushed
the
fix/snackbar_show_on_mouse_focus
branch
from
03415c9 to
d56de98
Compare
Fix Box layout seeking; select single result event.
obulat
force-pushed
the
fix/snackbar_show_on_mouse_focus
branch
from
d56de98 to
10a7079
Compare
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @zackkrida Excluding weekend1 days, this PR was ready for review 9 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
|
zackkrida
approved these changes
Aug 12, 2023
There was a problem hiding this comment.
LGTM. The question I asked about mobile can perhaps be handled, if necessary, in a separate issue.
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
Fixes #2742 by @obulat
Fixes #2124 by @zackkrida
Description
To fix the linked issues, this PR refactors the Audio search result components.
I realized that the components that display the audio result on the All content (
VAudioCell) and Audio search result pages (the inner part ofVAudioList) have the same functionality. Both are<li>list items, and both have to handle interaction and mouse down to send analytics events. So, I refactoredVAudioCelltoVAudioResultthat handles that, and can also set the correct audio track layout and size (rowif it's the Audio search page, andboxif its the All content search page).The new
VAudioResultcomponent is now used byVAudioList(the audio search results page and the related audio section on the single results page) and theVAllResultsGrid(the All content search results page).The snackbar handling is moved inside the
VAudioTrackcomponent. This allows us to not pass as many events up anymore.We also add
isSeekableprop touse-seekablecomposable to prevent seeking on box audio by arrow keys.Another change added here is to hide the related images using
brightness: 0%and remove themaxDiffPixelsthreshold in the snapshot comparisons. Previously, there was flakiness in these snapshots because sometimes the related images did not fully load and still had gray spans when we took the snapshots. This is not related to this PR, but since this PR was started because the Playwright failures in #2735, I decided to add it here, and rebase 2735 on this PR. This will help us see the changes in the external links in that PR.Testing Instructions
Test that the instructions snackbar is closed when you navigate away from search results. Go to the search page, navigate to the Audio item using keyboard, and close the snackbar by pressing Spacebar to play the audio. Then, press Enter to navigate to the single result page. The snackbar should be hidden.
Test that you cannot seek on the box layout. Go to the all content search page, and navigate to an audio item using your keyboard. Press arrow keys: you shouldn't be able to "seek" the audio. You should also see no
AUDIO_INTERACTIONanalytics events in the console.Check that audio interaction and
SELECT_SEARCH_RESULTevents are sent correctly for all audio tracks. When you click on audio to navigate to the single result page, you should get theSELECT_SEARCH_RESULTbut notAUDIO_INTERACTIONevent. When you click play or seek - you should get theAUDIO_INTERACTION, but noSELECT_SEARCH_RESULTevents.Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin