This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Use a modal for external sources on mobile #2078
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
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/2078 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. |
openverse-bot
added
✨ goal: improvement
|
Size Change: +385 B (0%) Total Size: 901 kB
ℹ️ View Unchanged
|
obulat
force-pushed
the
use_modal_for_external_search_on_mobile
branch
from
67a2072
to
6c3ff7a
Compare
dhruvkb
reviewed
Jan 6, 2023
This PR is for external sources under the search results. I've added the screenshot to the PR description. |
|
@panchovm could you review this for design? |
|
The overall implementation looks good, but I think it is based on an old mockup version. The spacings and buttons are from depracated components of the Design Library, so it is my duty to bring up an updated solution. Despite the design comment, I noticed an unintentional zoom-in bug. Put attention in the background of this screen recording CleanShot.2023-01-12.at.18.34.39.mp4Should I share a design ticket with an updated version of this modal or share the mockup here? The three improvement aspects are modal width, popover item, and text styles. |
|
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @panchovm Excluding weekend1 days, this PR was updated 4 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 |
obulat
force-pushed
the
use_modal_for_external_search_on_mobile
branch
2 times, most recently
from
6ced6a4
to
7c75b4a
Compare
fcoveram
approved these changes
Jan 31, 2023
|
@obulat I'm noticing one peculiarity in this PR, if I resize my browser to a mobile width I'm no longer able to scroll the page. Could merging in the recent changes to main fix this? |
Bind `attrs` to the actual popover/modal element instead of the parent that's not-visible
obulat
force-pushed
the
use_modal_for_external_search_on_mobile
branch
from
7c75b4a
to
03bf85e
Compare
|
Playwright Failure Test Results It looks like some of the Playwright tests failed. You can download the Playwright trace https://github.com/WordPress/openverse-frontend/actions/runs/4061257032 Read more about how to use this artifact here: https://github.com/WordPress/openverse-frontend/blob/main/test/playwright/README.md#debugging |
Were you resizing the browser to mobile width when the external sources popover is open or closed? |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 #734 by @panchovm
Description
This PR uses a modal for mobile external sources. To make this possible, it also adds another modal variant that is always centered and fits the content.
If the external sources button focus ring is incorrect, it will be fixed in a separate PR that will fix buttons focus rings.
Testing Instructions
Search for something and scroll down to see the external sources form. When you're on a mobile screen, you should see a modal if you click on the External sources button.
Checklist
Update index.md).main) ora parent feature branch.
errors.
Developer Certificate of Origin
Developer Certificate of Origin