Add VCollectionHeader component
#2734
Conversation
obulat
force-pushed
the
add/VCollectionHeader_component
branch
from
382f654
to
f19d4e1
Compare
github-actions
bot
added
🧱 stack: documentation
openverse-bot
added
🟨 priority: medium
|
Size Change: +3.19 kB (0%) Total Size: 878 kB
ℹ️ View Unchanged
|
|
Full-stack documentation: https://docs.openverse.org/_preview/2734 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
force-pushed
the
add/VCollectionHeader_component
branch
from
f19d4e1
to
0eef2b7
Compare
obulat
requested review from
AetherUnbound,
sarayourfriend and
fcoveram
and removed request for
sarayourfriend
| <div class="flex flex-col gap-y-6 lg:gap-y-8"> | ||
| <div class="flex flex-col gap-4 lg:flex-row lg:justify-between"> | ||
| <div class="flex items-center gap-x-2"> | ||
| <VIcon :name="iconName" :size="isDesktop ? 10 : 6" /> | ||
| <h1 class="text-3xl font-semibold leading-6 lg:text-6xl lg:leading-10"> | ||
| {{ title }} | ||
| </h1> |
There was a problem hiding this comment.
In md I envision a white gap in the top-right area for most cases. To avoid that, I think we should try changing to flex-row from md instead of lg .
|
Drafting while requested changes are worked on. |
obulat
force-pushed
the
add/VCollectionHeader_component
branch
2 times, most recently
from
ab5cd5c
to
bf83e35
Compare
|
I updated the styles to match the mockups better, and to go from a row to a column starting from the |
There was a problem hiding this comment.
This looks excellent so far, but there are a few things that need changing and clarification before I can approve. I've requested blocking changes to the story (change the id control's name) and the event configuration. I've suggested non-blocking but strongly recommended changes to the props for the component. Lastly, I've added a rather long question about how best to manage translation of the subtitle for each collection type. This didn't occur to me during design reviews but is rather significant if we're going to do it correctly. I suggested a workaround that might be a good stop-gap if we want to avoid blocking on this complexity. The workaround would need to be implemented alongside the prop changes.
| /** | ||
| * The label showing the result count, to display below the title. | ||
| */ | ||
| resultsLabel: { | ||
| type: String, | ||
| required: true, | ||
| }, |
There was a problem hiding this comment.
Passing the result label manually like this results in the storybook example not quite matching the designs, but only be consequence of the story configuration. The creator design has different text than the other collections:
If we changed the component to accept a configuration object on the collection prop where collection could be a union of the following interfaces:
interface BaseCollection {
type: unknown
mediaType: MediaType
resultCount: number
title: string
}
interface TagCollection extends BaseCollection {
type: 'tag'
}
interface SourceCollection extends BaseCollection {
type: 'source'
url: string
}
interface CreatorCollection extends BaseCollection {
type: 'creator'
source: string
url: string
}Then the component should select the correct template string and interpolate title, resultCount, mediaType and, if a creator, source.
There are probably some linguistic difficulties here though that might warrant passing individual source names to the translators... Some sources really need a definite article and others do not. Maybe leaving the definite article out is okay in English for some. "10000 results from Museum of Modern Art" reads clumsy to me. "10000 results from the Museum of Modern Art" would be better. But "10000 results from the NASA" is incorrect. In Portuguese the definite article is always included in the word "from" ("10000 resultados do Museum of Modern Art"). However, even then, the grammatical gender should be considered. If we had a source "Biblioteca Nacional do Brasil", the grammatical gender is feminine and it should be "10000 resultados da Biblioteca Nacional do Brasil".
I wonder if everywhere we are going to interpolate a source name into a sentence, whether we need to create a translation string for it, like automatically generating certain translations from the stats endpoints. We wouldn't translate the source names themselves, necessarily (though that may be appropriate in some cases, and transliterating source names could be useful for languages with non-Latin script), but it would still be necessary context for minimally correct translation in some languages. It would be jarring to read "10000 resultados do Public Library of New York" for an adult with English literacy, but all the more so (and potentially incomprehensible) for a child, e.g., in an educational context. We can't do anything about creator names, of course, but the list of sources is trivially obtainable and limited (unlike creator names of which there are probably hundreds of thousands and most cannot or shouldn't be translated or transliterated anyway).
If we're going down the road of creating sentences from source names, however, it does seem like something that we could consider in an entirely separate issue. I don't know exactly how adding those strings to en.json5 would work, if we could do so reliably, etc. We could go with a naive approach now that avoids needing to do any translations... maybe "10000 images - Biblioteca Nacional do Brasil", "10000 tagged images", and "10000 images - sarayourfriend, Flickr" (not concrete suggestions).
In either case, I think passing the resultsLabel in this way sort of invites accidental misuse or unintentionally duplicating similar translation strings.
There was a problem hiding this comment.
Great catch and suggestions.
Translating source names sound interesting, we can certainly can create an improvement issue to provide better context. So far, I like the following suggestion for the sentence.
10000 images - sarayourfriend, Flickr
Although I would replace the dash with a period to end up with something like this: 10000 images. sarayourfriend, Flickr
There was a problem hiding this comment.
@sarayourfriend, I started to implement changes to the label here, and then realized that it needs more props for some of the collection kinds (media type to show whether it's "... images" or "...audio files", results count, etc.). It's better to create the label in the parent component (the collection page that already has all the necessary information)
There was a problem hiding this comment.
That's why I suggested the expanded collection type, to encode the required information for each collection type. I generally have a preference for logic being as "low" in the component tree with higher components only responsible for overall layout, but where you think it would be best to do. Anywhere we put it will include the same complexity.
frontend/src/types/analytics.ts
Outdated
| /** The permalink to the source's homepage */ | ||
| url: string |
There was a problem hiding this comment.
How does url get used for filtering? For creator links, it will be a unique URL. Does it need to be the creator link? Could it just be the source slug? That would be easier to filter on most of the time.
frontend/src/types/analytics.ts
Outdated
| /** The unique ID of the media */ | ||
| id: string |
There was a problem hiding this comment.
Can we rename this to collectionType? The comment doesn't appear to be related to this event either.
| /** The unique ID of the media */ | |
| id: string | |
| /** The type of collection, either "source" or "creator" (tags do not have external links) */ | |
| collectionType: "source" | "creator" |
There was a problem hiding this comment.
The id property here should be removed, and collectionType is not necessary.
This event is fired when the user clicks on the external source link on the source collection page (the black button in the snapshots here).
When the user clicks on the creator's external link, a different event (VISIT_CREATOR_LINK) is fired.
There was a problem hiding this comment.
We could probably replace the url with the source slug - to make sure that if we ever change the URL we link to, the event is still the same.
| id: { | ||
| options: ["tag", "source", "creator", "source-with-long-name"], | ||
| control: { type: "select" }, | ||
| }, |
There was a problem hiding this comment.
id is a confusing name for this. I didn't understand what it did in the story controls until reading the code. example-collection would be clearer. Note: changing the name here requires updating the Template story implementation as well.
I wonder if we need two different stories, though? The all collections story feels entirely appropriate for the default story. It isn't a huge component that is a drag to use if all the examples are rendered at a single time.
| for (const collection of [ | ||
| "tag", | ||
| "creator", | ||
| "source", | ||
| "source-with-long-name", | ||
| ]) { |
There was a problem hiding this comment.
If we use the all collections story for this test instead then we don't need individual tests here. Is that worse for testing, though? I guess this way if only the tag changes then we have a tag-specific test that catches that change, rather than needing to dig through the diff to identify where the change is coming from.
There was a problem hiding this comment.
I refactored the story so that it's a single story for all 4 cases.
There was a problem hiding this comment.
It looks great. Just two comments:
- For the three variants, the horizontal layout in
mdis correct, buth1should follow the same style as inxs / sm - In Source, the results sentence should say
10000 images provided by this sourceinstead of10000 images from <source name>. This was thought to avoid i18n conflicts.
I updated the |
obulat
force-pushed
the
add/VCollectionHeader_component
branch
from
b88b4ef
to
7de0899
Compare
obulat
force-pushed
the
add/VCollectionHeader_component
branch
2 times, most recently
from
5c1991e
to
9273642
Compare
There was a problem hiding this comment.
My suggestion about applying the horizontal layout to md looks great, but I didn't consider that a smaller font size would break the alignment due to a 40px icon. The outcome looks broken.
I tried different ideas, but the best solution is going back to the previous text style ("Heading 2") but keeping the horizontal composition. Sorry for not anticipating this.
Button alignment
As a minor detail, the horizontal layout pushes the black button to the edge and creates a disalignment when it's close to the big text. We can solve this by adding a margin-top: 4px to the button. Here is my test in Chrome inspector.
Since this is only for the horizontal layout (md / lg / xl / 2xl), the button margin should disappear in xs / sm.
Results sentence
And finally, would this suggestion be address in this PR?
|
@fcoveram, I reverted the breakpoint change for the h1 style (it's |
frontend/src/components/VCollectionHeader/VCollectionHeader.vue
Outdated
Show resolved
Hide resolved
|
@obulat I was waiting for a final verdict on the sub-heading templating here: #2734 (comment) Can you confirm the plan? I'll review on Monday if this PR is ready to go, but then I think we'll need a new issue to record the changes/complexities of that sub-heading. |
Thank you, @sarayourfriend . The plan is to move the discussion of the changes/complexities of the sub-heading to #2774 that will create the subheading, translate it and pass as a prop to |
There was a problem hiding this comment.
LGTM!
Just two things:
- Might be worth update the sub-heading in the story to be a generic sub-heading text like "example sub-heading" rather than approximating the eventual designs, primarily to prevent confusion about why the story does not match the eventual designs or why the implementation on the final page doesn't match the story either.
- Something strikes me as unbalanced about the mobile button @fcoveram. Some screenshots for reference below. I don't know if this is because the story has all the different types all in a row (which will never happen in real life) and each of their buttons are slightly different widths (due to different text inside), but I think I do also perceive this unbalanced issue on the Figma designs as well. Particularly with a longer title and full-width image and audio results, the button seems out of place, somehow on the xs size. On the sm size it looks weird for it to be full-width, though.
With the story's grey background removed
Full width on xs size
Full width on sm size
Not sure what the solution is, or if it needs a change at all, even. But thought I would mention it just in case.
In any case, it doesn't need any changes in this PR, it can be addressed in a follow up issue if necessary.
Good work @obulat
obulat
force-pushed
the
add/VCollectionHeader_component
branch
from
189c051
to
59a26d5
Compare
|
Thanks for the suggestion @sarayourfriend; that section kept me iterating different ideas a lot. Unfortunately, one spacing solution doesn't work for horizontal and vertical layouts. But in the future, we can try showing the view name next to the icon on 
|
Fixes
Fixes #2729 by @obulat
Description
This PR adds the component that displays the collection icon, its title, external link (if available) and the number of results in the collection.
This component is not used anywhere yet, so you can only see it in the Storybook.
Testing Instructions
Check out the rendered Story. It should match the Figma designs ( creator desktop and mobile, source desktop and mobile, tag desktop and mobile).
Clicking on the external link button sends an analytics event, but since the build is "production", you cannot see the event details logged in the console, only the warning that Plausible was disabled.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin