Update the external search modal and items #2860
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
🟨 priority: medium
obulat
added
🕹 aspect: interface
|
Size Change: -1.6 kB (0%) Total Size: 873 kB
ℹ️ View Unchanged
|
obulat
force-pushed
the
update/external_search_modal_items
branch
2 times, most recently
from
2e86b6f
to
36b0059
Compare
obulat
commented
Aug 23, 2023
fcoveram
requested changes
Aug 24, 2023
Comment on lines
66
to
74
| <VCloseButton | ||
| size="small" | ||
| icon-size="medium" | ||
| variant="filled-white" | ||
| :label="$t('modal.close')" | ||
| @close="close" | ||
| /> |
There was a problem hiding this comment.
I see the button in medium size (40px) instead of small (32px), it might be the icon-size property. It also should be transparent-gray instead of filled.
| </VButton> | ||
| </template> | ||
| <template #top-bar="{ close }"> | ||
| <header class="flex items-center justify-between pe-4 ps-7 pt-4"> |
There was a problem hiding this comment.
In xs paddings should be left: 28px; top and right: 20px, while in sm and above should be left: 36px; top and right: 28px.
| /> | ||
| </header> | ||
| </template> | ||
| <VExternalSourceList class="flex flex-col" :search-term="searchTerm" /> |
There was a problem hiding this comment.
Icon size of external link item needs to be like VIconButton in size: small, but not interactable. This is for consistenty with the new popover item requested in #2839.
There was a problem hiding this comment.
I'm not sure I correctly understand what "not interactable" means for the external link: when you hover over it, the whole button becomes "hovered". You cannot click on the icon itself, all clicks are button clicks.
There was a problem hiding this comment.
Sorry for not explaining it correctly.
The popover item in #2839 has an optional icon on the right side that in some cases you can interact with it. Here is an example.
Although the above has not been implemented yet, the new style was thought to allow this internal action.
When the action is not needed, like in this case where ↗ only indicates a link type, the icon area is the same (32px for the container, 24px as icon size) to keep consistency across all popover items. Here is a screenshot of popover item in wireframe.
The smallest box area is the default icon size of 24px, and the container is like VIconButton in 32px (size: small).
There was a problem hiding this comment.
At first when I read this note, I thought maybe we should create a separate component in code. However, because they have different functions (the external icon is a "decoration" that has no functionality and the remove "cross" icon is actually an icon button that should have some click handlers) - I think it's best to leave the "external icon" here as VIcon. I set the icon size to 5, which makes the icon itself around 16px high - just like in the mockups. I think the size of the icon now matches the designs. Could you please confirm, @fcoveram ?
There was a problem hiding this comment.
because they have different functions (the external icon is a "decoration" that has no functionality and the remove "cross" icon is actually an icon button that should have some click handlers) - I think it's best to leave the "external icon" here as VIcon
That makes sense to me. But VIcon needs to be size: 6 (24px) and placed inside a 32px box to look balanced. Without the 32px container, it looks broken.
Here is a design test where the color-filled area is the VIcon in size 6, and the border-only area is the 32px container.
I reviewed the PR's last changes and still see the icon in 20px. Here is a gif of what I see.
There was a problem hiding this comment.
VIcon in size 8 is a 32px element that scales up the icon, whereas the above refers to a container area to balance spaces between popover items while keeping the icon in 24px.
obulat
force-pushed
the
update/external_search_modal_items
branch
from
0b2e457
to
03eba7a
Compare
obulat
commented
Aug 24, 2023
| @@ -226,7 +226,6 @@ export default { | |||
| }, | |||
| borderRadius: { | |||
| inherit: "inherit", | |||
| md: "0.5rem", | |||
There was a problem hiding this comment.
I didn't realize that rounded-lg is actually border-radius: 0.5rem when I added this new value, md. Since md is redundant now, I removed it and replaced it in the components with rounded-lg
obulat
force-pushed
the
update/external_search_modal_items
branch
from
03eba7a
to
6531eb8
Compare
fcoveram
approved these changes
Aug 25, 2023
obulat
force-pushed
the
update/external_search_modal_items
branch
from
6f04267
to
79fb572
Compare
AetherUnbound
approved these changes
Aug 29, 2023
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 #460 by @fcoveram
Description
This PR updates the external search items modal according to the updated designs.
VModalto simplify the component, instead of handling the modal behavior manually.xs, and has a max-width set to 400px for breakpointssmand aboveThe
VModalContenthad a bug where thehideprop in the top-bar slot was not passed to theVModal, so it was impossible to close the modal using the close button. This PR fixes that, too.Designs
Figma file: Metasearch improvement.
Testing Instructions
Search for images or audio, scroll to the bottom and click on the "External sources" button. The modal should match the designs, should be closable, and should update sizes if you change the screen width. You should be able to open the external sources links.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin