Add source and provider definition tooltips #3407
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
openverse-bot
added
the
🚦 status: awaiting triage
|
Size Change: +15.6 kB (+2%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
obulat
force-pushed
the
add/source_provider_definitions
branch
from
11f5aa6
to
ca3307b
Compare
obulat
added
🌟 goal: addition
obulat
force-pushed
the
replace-popper-with-floating-ui
branch
from
0fd143e
to
e42599b
Compare
obulat
force-pushed
the
add/source_provider_definitions
branch
from
ca3307b
to
cdc5bf4
Compare
obulat
force-pushed
the
replace-popper-with-floating-ui
branch
4 times, most recently
from
66a8fb0
to
ba5db45
Compare
obulat
force-pushed
the
add/source_provider_definitions
branch
2 times, most recently
from
697941e
to
cfbb408
Compare
openverse-bot
added
the
💻 aspect: code
obulat
changed the title
obulat
force-pushed
the
add/source_provider_definitions
branch
from
cfbb408
to
b12da11
Compare
obulat
force-pushed
the
add/source_provider_definitions
branch
from
b12da11
to
de98800
Compare
openverse-bot
added
the
💻 aspect: code
AetherUnbound
approved these changes
Dec 19, 2023
fcoveram
previously requested changes
Dec 20, 2023
| <template> | ||
| <dt> | ||
| {{ $t(datum.label) }} | ||
| <VTooltip placement="top" :describedby="describedby" class="ms-2"> |
There was a problem hiding this comment.
I think this class adds a margin-left of 8px instead of 4px. ms-1 should fix it I think.
Comment on lines
2
to
12
| <dl v-if="isSm" class="metadata grid gap-8" :style="columnCount"> | ||
| <div v-for="datum in metadata" :key="`${datum.label}`"> | ||
| <dt class="label-regular mb-1 ps-1">{{ $t(datum.label) }}</dt> | ||
| <div v-for="datum in metadata" :key="datum.label"> | ||
| <VSourceProviderTooltip | ||
| v-if="tooltipId(datum)" | ||
| :describedby="tooltipId(datum)" | ||
| class="label-regular mb-1 flex flex-row ps-1" | ||
| :datum="datum" | ||
| /> | ||
| <dt v-else class="label-regular mb-1 flex flex-row ps-1"> | ||
| {{ $t(datum.label) }} | ||
| </dt> |
There was a problem hiding this comment.
The info icon is not horizontally centered. I tried the following in Chrome inspector, and it looked good:
- Added
items-center - Just in
xs, I added a padding of 4px to top, right, and bottom indtelement. For bigger breakpoints,ps-1works correctly
There was a problem hiding this comment.
Sorry, @fcoveram , I didn't mean to re-request your review yet
obulat
force-pushed
the
add/source_provider_definitions
branch
from
0e776d2
to
f3a88b0
Compare
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @fcoveram Excluding weekend1 days, this PR was ready for review 19 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 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
|
dhruvkb
approved these changes
Jan 15, 2024
Comment on lines
+42
to
+46
| return i18n.t( | ||
| props.datum.name === "source" | ||
| ? "mediaDetails.sourceDescription" | ||
| : "mediaDetails.providerDescription" | ||
| ) |
There was a problem hiding this comment.
Suggested change
| return i18n.t( | |
| props.datum.name === "source" | |
| ? "mediaDetails.sourceDescription" | |
| : "mediaDetails.providerDescription" | |
| ) | |
| return i18n.t(`mediaDetails.${props.datum.name}Description`) |
There was a problem hiding this comment.
Static keys can be checked by vue-i18n-extract`. It does not handle dynamic keys like this well.
obulat
force-pushed
the
add/source_provider_definitions
branch
from
f3a88b0
to
cca1c12
Compare
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
force-pushed
the
add/source_provider_definitions
branch
from
cca1c12
to
3c29195
Compare
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
Fixes #2775 by @fcoveram
Fixes #2840 by @fcoveram
Description
Adds the tooltip that opens when you focus or hover over the (i) symbol next to the Provider or Source in the single result page:
I just realized that the provider definition would not be correct when/if we extract sources from Europeana: here, Europeana is an aggregator which allows us to search for images, but the images themselves are hosted on the source sites, not Europeana.
The tooltip
Here are the Figma links to:
hoverstateAccessibility considerations
WAI ARIA guidelines
Testing Instructions
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin