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

Display generated tags separately #4291

Merged
merged 10 commits into from
Jun 24, 2024
Merged

Display generated tags separately #4291

merged 10 commits into from
Jun 24, 2024

Conversation

obulat
Copy link
Contributor

@obulat obulat commented May 8, 2024

Fixes

Fixes #4192 by @fcoveram

Description

This PR adds a separate display of machine-generated tags and tags provided by sources on the single result page following the Figma mockups.

Based on @fcoveram's suggestion, if the item only has source tags, the tags section doesn't have a visible heading.

I started to write the implementation plan for the frontend feature, but realized that this can be done in a single PR. As the project lead, do you think we need a frontend IP, @AetherUnbound?

This PR is blocked on #4379 because it has a link to the "About tags" page, but the page is not ready.
Another blocker is the text for this link. Currently, the link says "What is this?"; suggestions for the text are welcome.

The PR can be reviewed but should not be merged before #4379 is done.
#4379 was merged into this PR. Please also review the wording here.

Testing instructions

Run the app using just frontend/run dev (running dev:only would not update the English strings, so you would need to run dev at least once)
Go to http://localhost:8443/image/a487f4eb-ce05-43e1-acae-73c4ab090cc9 to see how machine-generated and source tags are displayed.

@obulat obulat self-assigned this May 8, 2024
@obulat obulat requested a review from a team as a code owner May 8, 2024 14:13
@obulat obulat requested review from zackkrida and AetherUnbound May 8, 2024 14:13
@github-actions github-actions bot added the 🧱 stack: frontend Related to the Nuxt frontend label May 8, 2024
@openverse-bot openverse-bot added 🟨 priority: medium Not blocking but should be addressed soon ✨ goal: improvement Improvement to an existing user-facing feature 🖼️ aspect: design Concerns related to design labels May 8, 2024
@obulat obulat force-pushed the add/generated-tags-display branch from 720dbc2 to a4a043a Compare May 8, 2024 14:40
@zackkrida
Copy link
Member

@obulat this looks great with the exception of an (existing) spacing issue that becomes more apparent when we add a text label above the tags.

image

Observe that the "Source" and "Source Tags" text labels are not in alignment. "Source" is 0.25rem to the right of "Source Tags".

I think we just need to remove horizontal padding from the dd and dt elements, and that this will fix the pages to match the mockups.

@AetherUnbound
Copy link
Collaborator

I started to write the implementation plan for the frontend feature, but realized that this can be done in a single PR. As the project lead, do you think we need a frontend IP, @AetherUnbound?

I think an IP would still be good, even just as a decision log for some of the questions/points that were made in the project proposal. It would not have to be a big document IMO!

Copy link
Collaborator

@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.

This looks good when I run it locally! One question about some other information we have available for the generated tags.

@obulat obulat marked this pull request as draft May 9, 2024 04:19
@obulat obulat force-pushed the add/generated-tags-display branch from c8b39e0 to f262ee9 Compare May 20, 2024 16:24
@fcoveram
Copy link
Contributor

To @zackkrida's point

Observe that the "Source" and "Source Tags" text labels are not in alignment. "Source" is 0.25rem to the right of "Source Tags".

If I recall correctly, we intentionally added this spacing to avoid cropping links' focus ring. In this case, the flickr label.

We can add the same spacing in this section to make it look align. Or perhaps, and correct me if I'm wrong, investigate how to fix this site-wise issue and stop us from adding empty spacings that could break the layout.

@obulat obulat force-pushed the add/generated-tags-display branch 3 times, most recently from cc4dabd to 12bdfa4 Compare May 29, 2024 06:45
@obulat
Copy link
Contributor Author

obulat commented May 29, 2024

We can add the same spacing in this section to make it look align. Or perhaps, and correct me if I'm wrong, investigate how to fix this site-wise issue and stop us from adding empty spacings that could break the layout.

I added a negative margin to the "Source" or "Provider" (whichever comes first) to fix this. You can see in the snapshots that they are aligned now.

Now, I looked at the snapshots again, and I feel like the tags headings are fine when there are both source and generated tags. However, when there are only source tags, the "Source tags" heading is not very clear. Why are they called "source" tags? And since the "What it is?" link is only available on the Generated tags, it is not possible to navigate to the explanation page. What do you think, @fcoveram?

@fcoveram
Copy link
Contributor

I think we can safely remove the "Source tags" heading when there are no machine-generated tags. The pill set in the media details area is common content in stock and other similar services and does not need further explanation.

@obulat obulat force-pushed the add/generated-tags-display branch from 97fd532 to 15cb517 Compare May 29, 2024 08:01
@obulat
Copy link
Contributor Author

obulat commented May 29, 2024

I think we can safely remove the "Source tags" heading when there are no machine-generated tags. The pill set in the media details area is common content in stock and other similar services and does not need further explanation.

Updated in 15cb517. I also added a screen-reader-only "Tags" heading.

@obulat obulat marked this pull request as ready for review May 29, 2024 08:08
@obulat obulat added the ⛔ status: blocked Blocked & therefore, not ready for work label May 29, 2024
@obulat obulat force-pushed the add/generated-tags-display branch 2 times, most recently from ad2eb8b to 2e40317 Compare May 29, 2024 13:40
@zackkrida
Copy link
Member

Currently, the link says "What is this?"; suggestions for the text are welcome.

@obulat I think the classic "Learn more" would be appropriate here.

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.

LGTM. I'll write the copy for the page next week. I made sure to test that collapsible tags are still working properly; no issues there.

@obulat obulat force-pushed the add/generated-tags-display branch from 6b64053 to 2de2e9e Compare June 4, 2024 10:42
@obulat obulat mentioned this pull request Jun 11, 2024
@obulat obulat force-pushed the add/generated-tags-display branch from dc6dd3f to d0c23c3 Compare June 11, 2024 10:28
@obulat obulat removed the ⛔ status: blocked Blocked & therefore, not ready for work label Jun 11, 2024
@zackkrida zackkrida requested a review from fcoveram June 18, 2024 19:58
@zackkrida
Copy link
Member

Hey @fcoveram could you look at this again and let us know what you think? Thanks 😄

@fcoveram
Copy link
Contributor

I will take a look at this tomorrow morning 🙏 sorry for the delay

@openverse-bot
Copy link
Collaborator

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

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

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

Copy link
Contributor

@fcoveram fcoveram left a comment

Choose a reason for hiding this comment

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

All looks good 🚀 🌟

@obulat obulat force-pushed the add/generated-tags-display branch from d0c23c3 to 26cdcc8 Compare June 24, 2024 15:04
@obulat obulat merged commit 50fe56e into main Jun 24, 2024
43 checks passed
@obulat obulat deleted the add/generated-tags-display branch June 24, 2024 15:17
@obulat obulat mentioned this pull request Jun 25, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🖼️ aspect: design Concerns related to design ✨ goal: improvement Improvement to an existing user-facing feature 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: frontend Related to the Nuxt frontend
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Displaying machine-generated content
5 participants