Create VTag component #3137
Create VTag component #3137
Conversation
openverse-bot
added
🟨 priority: medium
There was a problem hiding this comment.
This is a great start, @ngken0995!
I've added several requests for changes in the inline comments.
I also realized that it's not too difficult to add this new component to the VMediaTags.vue component under the additional_search_views feature flag. If you want, you can add those changes, too:
In the VMediaTags script, add:
setup() {
const featureFlagStore = useFeatureFlagStore()
const additionalSearchViews = computed(() => featureFlagStore.isOn("additional_search_views"))
return {
additionalSearchViews
}
And in the template in the same file add VTag if the flag is on:
<ul v-if="tags.length" class="flex flex-wrap gap-2">
+ <VTag v-if="additionalSearchViews" v-for="(tag, index) in tags" :key="index" href="/" :title="tag.name"/>
+ <VMediaTag v-else v-for="(tag, index) in tags" :key="index" tag="li">{{
tag.name
}}</VMediaTag>
</ul>
| </template> | ||
|
|
||
| <script lang="ts"> | ||
| import { defineComponent } from "@nuxtjs/composition-api" |
There was a problem hiding this comment.
| import { defineComponent } from "@nuxtjs/composition-api" | |
| import { defineComponent } from "vue" |
We are trying to move away from @nuxtjs/composition-api wherever possible: this will make the transition to Nuxt 3 easier. This is why we only use the defineComponent imported from @nuxtjs/composition-api on pages. On a component like this, it's better to import it from vue.
| }) | ||
| </script> | ||
|
|
||
| <style scoped></style> |
There was a problem hiding this comment.
| <style scoped></style> |
| name: "VTag", | ||
| components: { VButton }, | ||
| props: { | ||
| title: { type: String }, |
There was a problem hiding this comment.
This prop has to be required, too, because a tag without the title does not make sense.
| }, | ||
| }) | ||
|
|
||
| <Meta title="Components/VTag" component={VTag} /> |
There was a problem hiding this comment.
This story should have args and argTypes set up: this will allow us to set the title and the href to test in the storybook. You can see examples of how to set this up in other .mdx files.
|
@obulat Here is a video of how it works on the webpage: https://youtu.be/LpomDHUhY8U. I have made changes to VTag and VMediaTags. I'll start working on VTag Storybooks. |
|
@obulat I tried running How can I resolve this? |
There was a problem hiding this comment.
Video looks nice, but there are duplicated tags now. I added suggestions on what to do to remove the old VMediaTags when additional_search_views feature flag is on.
As for the error you mention, I'm not sure what causes it. Maybe your local folder needs permissions to be adjusted?
| @@ -1,25 +1,45 @@ | |||
| <template> | |||
| <ul v-if="tags.length" class="flex flex-wrap gap-2"> | |||
| <div v-if="additionalSearchViews"> | |||
| <VTag | |||
| v-for="(tag, index) in tags" | |||
There was a problem hiding this comment.
| v-for="(tag, index) in tags" | |
| v-if="additionalSearchViews" | |
| v-for="(tag, index) in tags" |
You don't need to wrap the tags in a separate div. Also, we need to hide the VMediaTags if additionalSearchViews is true.
| href="/" | ||
| :title="tag.name" | ||
| /> | ||
| </div> | ||
| <VMediaTag v-for="(tag, index) in tags" :key="index" tag="li">{{ |
There was a problem hiding this comment.
If you add v-else to VMediaTag here, it will be hidden when additionalSearchViews is true, so there'll be no duplication of tags.
|
@obulat I'm getting a eslint error for putting |
Right, sorry, @ngken0995, I didn't check the suggestion locally before. You can wrap the tags with |
There was a problem hiding this comment.
I updated the testing instructions for the other reviewers.
Thank you for your contribution, @ngken0995 , the tags look great!
There was a problem hiding this comment.
The tags work well, thanks for the contribution @ngken0995!
While I do think there is a possibility to improve VTag further (by forwarding all props to the inner VButton, using a slot for the content instead of a prop, adding accessible labels to the links etc.), I won't block the merge of this PR and we can consider those improvements in a separate issue.
Please let me know if you will create one. I will be interested in working on it. |
|
Created #3190 for further refinements, cc: @ngken0995. |
Fixes
Fixes #2773 by @obulat
Description
Created a VTag component for tags to be clickable. It is visible on the single result page when the
additional_search_viewsflag is on.Testing Instructions
Added by @obulat:
Go to
/preferencesand turn on theadditional_search_viewsflag. Then, open a single result page. You should see "clickable" tags that match the Figma designs. The tags don't have the correcthrefyet, so nothing happens when you click on a tag.Also, run Storybook (
just frontend/run storybook) and check theVTagstory.Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin