Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation Plan: Machine-generated tags on the frontend #4302

Merged
merged 7 commits into from
May 24, 2024

Conversation

obulat
Copy link
Contributor

@obulat obulat commented May 10, 2024

Due date:

2024-05-24

Assigned reviewers

Both of you were chosen for your familiarity with the project and the frontend. Input from @fcoveram is also necessary from the design point of view: whether the proposed approach is feasible and good for the UX.

Description

Resolves #4039

Current round

This discussion is following the Openverse decision-making process. Information
about this process can be found
on the Openverse documentation site.
Requested reviewers or participants will be following this process. If you are
being asked to give input on a specific detail, you do not need to familiarise
yourself with the process and follow it.

This discussion is currently in the Decision round.

The deadline for review of this round is 2024-05-28

@obulat obulat requested a review from a team as a code owner May 10, 2024 15:53
@obulat obulat requested review from krysal, AetherUnbound and zackkrida and removed request for krysal May 10, 2024 15:53
@github-actions github-actions bot added the 🧱 stack: documentation Related to Sphinx documentation label May 10, 2024
@openverse-bot openverse-bot added 🟨 priority: medium Not blocking but should be addressed soon 🌟 goal: addition Addition of new feature 📄 aspect: text Concerns the textual material in the repository labels May 10, 2024
Copy link

Full-stack documentation: https://docs.openverse.org/_preview/4302

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.

New files ➕:

Copy link
Contributor

@AetherUnbound AetherUnbound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for codifying this all into a document! I support the suggested approach broadly, just a few clarification pieces before a decision is made here.

Comment on lines +75 to +76
source and the generated tags. However, each section individually will still be
normalized to prevent duplicates.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, this is a great way to handle that!

Comment on lines 83 to 84
there could be more tag sections. The tags in each subsection will be ordered by
their accuracy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great call, I really like this approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the need behind this decision? I don’t envision users needing to distinguish between providers to make a better search decision. Showing the provider assumes that users know the difference between them, like how data is processed and therefore the output.

I would go with something simpler for this release, and once we reach users when #3559 begins, inquiry how users relate to machine-generated metadata during their searches.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a lot of sense, @fcoveram. I'll update the IP to use alternative 1 as the selected choice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be best not to sub-divide by provider; later, we could follow-up and display that information some other way in the future.

No infrastructure changes will be necessary, we will only update the single
result pages of the Nuxt frontend.

## Alternatives
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for including the alternatives!


The tags will be displayed in two sections: "Source tags" and "Generated tags".

For the machine-generated tags, we want to add a link to the page that describes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is missing a bit of information at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out. This was actually extra that should have been removed.

Comment on lines 193 to 199
## Risks

<!-- What risks are we taking with this solution? Are there risks that once taken can’t be undone?-->

## Prior art

<!-- Include links to documents and resources that you used when coming up with your solution. Credit people who have contributed to the solution that you wish to acknowledge. -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we don't have any information for these sections, we can remove them.

@zackkrida
Copy link
Member

@obulat I see there are a few places with missing text still, I can review this more deeply after those are addressed. I am also positive about the general approach outlined so far :)

@obulat obulat force-pushed the add/machine-generated-tags-frontend-ip branch from e735032 to 33af2a0 Compare May 15, 2024 18:28
@obulat
Copy link
Contributor Author

obulat commented May 15, 2024

@obulat I see there are a few places with missing text still, I can review this more deeply after those are addressed. I am also positive about the general approach outlined so far :)

@zackkrida, I updated the proposal, and it's ready for review

@zackkrida
Copy link
Member

I will review this today.

Copy link
Contributor

@AetherUnbound AetherUnbound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, no blocking objections! This is so exciting!

Comment on lines 180 to 182
and the accuracy of the tag without any additional actions. We need to make sure
that the tag name and its accuracy are separated (such as with an invisible,
`sr-only`, dot) to make the screen readers add a pause between them.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great call-out!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're deciding not to go with this approach, do you mind changing the language (specifically We need to make sure...) to indicate that, if we were to choose this approach down the line, this is a consideration we'd need to take?

<!-- Choose two people at your discretion who make sense to review this based on their existing expertise. Check in to make sure folks aren't currently reviewing more than one other proposal or RFC. -->

- [ ] @zackkrida
- [ ] @AetherUnbound
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [ ] @AetherUnbound
- [x] @AetherUnbound

@openverse-bot
Copy link
Collaborator

Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR:

@zackkrida
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend1 days, this PR was ready for review 5 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

  1. Specifically, Saturday and Sunday.

  2. For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range.

Comment on lines 90 to 95
### Add "About tags" description page

We should add a page to explain the users the difference between where the
Openverse tags come from, the difference between the source and generated tags,
and a short description of the machine-generated tag providers. This page should
be linked from the Generated tags section.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with adding content to explain tag are generated, but I disagree with adding a 10th page in the content header.

The current header is quite long, and adding an extra link in the current set doesn't provide enough information about what "tags" is. Instead, I would address the content need as proposed here for #3564.

The page could be very simple: reuse the current grid and page style, and add a fixed TOC on the left to jump between sections. Since the section will be linked from the search page, the information is introduced with more context about what Openverse is about and how relates to sources and content.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote "similar to the sensitive content page" several times, and deleted it. I guess I should have left it in.

What I meant here was we should have a page explaining what the machine-generated tags are, but not add it to the header. The sensitive media page is just like that: you can only navigate it by typing the address in the URL bar, or by opening a sensitive media item and clicking on a link from that page. Do you think this would work, @fcoveram ?

Copy link
Contributor

@fcoveram fcoveram May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant here was we should have a page explaining what the machine-generated tags are, but not add it to the header.

"Add a page" resonated with me as adding it in the header as it's the only component where we add pages. But you're right, it doesn't mention the header at all.

@fcoveram
Copy link
Contributor

Very clear document. I left comments related to showing how tags are generated.

In short, I agree with the content need and embrace the "giving all the details" approach, but I think we can introduce the info with more context more smoothly.

slot in the `VTag` component will enable us to display the additional accuracy
information.

### Add "About tags" description page
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good 👍 at some point in the future we'll probably need to start thinking about the page hierarchy on openverse.org and what should be on docs.openverse.org or openverse.org. Also, if we thinks docs.openverse.org is appropriate for users or only contributors and API consumers.

Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed we should skip disambiguating providers from each other on first pass; otherwise LGTM.

@AetherUnbound AetherUnbound added the 🧭 project: implementation plan An implementation plan for a project label May 20, 2024
@obulat obulat self-assigned this May 21, 2024
@obulat
Copy link
Contributor Author

obulat commented May 21, 2024

Drafting to update

@obulat obulat marked this pull request as draft May 21, 2024 15:10
@obulat obulat force-pushed the add/machine-generated-tags-frontend-ip branch 2 times, most recently from 7d38ec9 to 5323d54 Compare May 23, 2024 14:20
@obulat obulat force-pushed the add/machine-generated-tags-frontend-ip branch from 5323d54 to 6216619 Compare May 23, 2024 15:19
@obulat obulat marked this pull request as ready for review May 23, 2024 15:19
Copy link
Contributor

@AetherUnbound AetherUnbound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small notes, but no blocking objections!

page to display this data in full. As a bonus, if a tag is generated by
multiple providers, the user will see that different providers generate the
same tag, and will be able to see the difference in accuracy between the
providers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to point 1, do you mind adding a note here on why this was not an ideal choice right now?

Comment on lines 180 to 182
and the accuracy of the tag without any additional actions. We need to make sure
that the tag name and its accuracy are separated (such as with an invisible,
`sr-only`, dot) to make the screen readers add a pause between them.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're deciding not to go with this approach, do you mind changing the language (specifically We need to make sure...) to indicate that, if we were to choose this approach down the line, this is a consideration we'd need to take?

Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @AetherUnbound's suggestions. LGTM

obulat and others added 6 commits May 24, 2024 12:26
Signed-off-by: Olga Bulat <obulat@gmail.com>
…lementation_plan_machine_generated_tags_frontend.md

Co-authored-by: Madison Swain-Bowden <bowdenm@spu.edu>
…lementation_plan_machine_generated_tags_frontend.md

Co-authored-by: Madison Swain-Bowden <bowdenm@spu.edu>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
@obulat obulat force-pushed the add/machine-generated-tags-frontend-ip branch from 6216619 to 7b7df91 Compare May 24, 2024 11:24
Signed-off-by: Olga Bulat <obulat@gmail.com>
@obulat obulat merged commit 87880a0 into main May 24, 2024
41 checks passed
@obulat obulat deleted the add/machine-generated-tags-frontend-ip branch May 24, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 aspect: text Concerns the textual material in the repository 🌟 goal: addition Addition of new feature 🟨 priority: medium Not blocking but should be addressed soon 🧭 project: implementation plan An implementation plan for a project 🧱 stack: documentation Related to Sphinx documentation
Projects
Status: Accepted
Status: 🤝 Merged
5 participants