Update VCollectionHeader.vue #3365
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
github-actions
bot
added
🧱 stack: documentation
openverse-bot
added
the
🚦 status: awaiting triage
|
Size Change: +902 B (0%) Total Size: 974 kB
ℹ️ View Unchanged
|
obulat
added
🛠 goal: fix
obulat
force-pushed
the
update/VCollectionHeader
branch
from
8f9d82b to
4e6b00b
Compare
github-actions
bot
added
the
🧱 stack: documentation
obulat
force-pushed
the
update/VCollectionHeader
branch
from
4e6b00b to
8cc0cf3
Compare
|
Full-stack documentation: https://docs.openverse.org/_preview/3365 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. |
AetherUnbound
approved these changes
Nov 17, 2023
Comment on lines
+831
to
+834
| audio: { | ||
| zero: "No audio files by this creator in {source}", | ||
| count: "{count} audio file by this creator in {source}|{count} audio files by this creator in {source}", | ||
| countMore: "Over {count} audio files by this creator in {source}", |
There was a problem hiding this comment.
These seem very similar, would it also be possible to interpolate the media type here too? Or is it just easier to define them separately?
There was a problem hiding this comment.
I think it's better to leave as much as possible fully written out because interpolation does not always work well for different languages, which might have very different pluralization and other grammar rules.
dhruvkb
approved these changes
Nov 18, 2023
| resultCountLabel: { | ||
| creator: { | ||
| audio: { | ||
| zero: "No audio files by this creator in {source}", |
There was a problem hiding this comment.
Is "track" a better word than "file" here? I would like us to identify a word that's more descriptive than "file" and use it consistently in the UI.
Signed-off-by: Olga Bulat <[email protected]>
obulat
force-pushed
the
update/VCollectionHeader
branch
from
8cc0cf3 to
af65ff8
Compare
Thank you for this comment, I did update the test to keep the number. The snapshots still had to be changed, though, due to the fact that I didn't add "over" in the original implementation, and instead of "Over 10000 results", it was just "10000 results" before 😢 |
6 tasks
8 tasks
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
Updated implementation of #2729 by @obulat
Description
This PR is extracted from #3140 to make them easier to review.
When adding Nuxt page for the tag/source/creator collections in #3140, I first tried to add a single dynamic page for all of them to prevent code duplication.
I used a dynamic catch-all path
_mediaType/_collection/_.vue, and thought that_mediaTypewould catch theimageoraudiopath parameter,_collectionwould catch the first part of the path (tagorsource), and_.vuewould be the catch-all that collects the rest of the path parameters:tag/tagName,source/sourceName,source/sourceName/creator/creatorName.However, this strategy catches more routes than expected. For instance, it would catch
/<localePrefix>/non-existent-page, and would return the English error page because the collection page was seeing the locale prefix as_mediaType, and path validation would fail because the locale prefix does not match the_mediaType(image/audio).This is why I added 4 pages that used less dynamic path parameters (
/image/tag/_tag.vue,/image/source/_.vue).Previously, I planned to use the page to compute all the necessary parameters, and have
VCollectionHeaderas a simple presentational component. However, now that there are 4 page component, moving the computation to the store and the component makes more sense.Now
VCollectionHeaderaccepts thecollectionParamsprop from the page component, and then computes the title, URL and the results label. There is a complication for the creator pages: we can only get the creator URL after we fetch the media, from the media item'screator_urlparam. This is why thecreatorUrlis still computed in the page (which handles media fetching) and is passed as a prop tocollectionParams.This PR also converts the
CollectionParamstype to a discriminated union type to make it easier to narrow the type when handling it.Testing Instructions
Check the Storybook to see that
VCollectionHeaderis rendered correctly: https://docs.openverse.org/_preview/3365/storybook/?path=/story/components-vcollectionheader--all-collectionsTo run the Storybook locally, you may need to run
just frontend/run test:storybookto run Storybook inside docker (it currently does not run locally on Mac due to ssl errors).Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin