This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Add homepage content switcher #2087
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
obulat
added
🟨 priority: medium
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/2087 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. |
|
Size Change: -7.8 kB (-1%) Total Size: 887 kB
ℹ️ View Unchanged
|
|
It looks great ✨ I just noticed the following minor details: Filter tab in drawer.From
Popover position and layoutIn
In narrow-height devices, the popover is cut off, as shown below.
Selecting a content typeBased on this comment, the drawer should hide the "See results" button and close itself once shifting to a new media.
On the other hand, it seems better to set the drawer's height to This layout comes from the search result drawer, but in that case, the height is a device's height percentage to allow scrolling filters. Item sizeI noticed that in both drawer and popover elements, the content type item is slightly bigger. I wonder if we took this approach to meet i18n requirements. If so, my believe is that label lengths won't reach the full width of its container; in the same way we did with the header's search input. If that is the case, the best would be finding a new spacing solution. The switcher will not change as often as adding new filters or any other similar menu displayed in a popover. This comment might go beyond this PR's scope, so please feel free to skip it and address this point in a different ticket. Even so, here are the specs. Size in drawerThe button has a height of
Size in popoverIn this case, item height is
|
obulat
force-pushed
the
add/homepage_content_switcher_popover
branch
2 times, most recently
from
7e19c1c to
3213e41
Compare
|
It doesn't need to happen in this PR, but should we have homepage snapshot test(s) which include the open popover? |
obulat
commented
Jan 15, 2023
obulat
force-pushed
the
add/homepage_content_switcher_popover
branch
from
d612f64 to
a3c1af2
Compare
7 tasks
obulat
force-pushed
the
add/homepage_content_switcher_popover
branch
from
91db330 to
1bc4c96
Compare
obulat
force-pushed
the
add/content_switcher_vr_tests
branch
from
cd67b43 to
027caae
Compare
obulat
force-pushed
the
add/homepage_content_switcher_popover
branch
from
1bc4c96 to
07643d3
Compare
obulat
force-pushed
the
add/homepage_content_switcher_popover
branch
from
cf89dba to
722978e
Compare
obulat
force-pushed
the
add/homepage_content_switcher_popover
branch
from
722978e to
3fcee75
Compare
dhruvkb
approved these changes
Jan 17, 2023
| @@ -93,6 +93,7 @@ module.exports = { | |||
| 40: "10.00rem", | |||
| 50: "12.50rem", | |||
| 64: "16.00rem", | |||
| 66: "16.25rem", | |||
There was a problem hiding this comment.
After a certain point (which we might have already crossed), it might make more sense to replace this with:
Object.fromEntries(Array.from({ length: 120 }, (_, i) => [i, `${i/4}rem`]))| @@ -18,7 +18,7 @@ | |||
| <VButton | |||
| data-item-group-item | |||
| :as="as" | |||
| class="group relative flex min-w-full justify-between py-2 hover:bg-dark-charcoal-10 focus:z-10" | |||
| class="group relative flex min-w-full justify-between border-0 py-2 hover:bg-dark-charcoal-10 focus:z-10" | |||
There was a problem hiding this comment.
Removing the border here makes this a special variant of Button that's only defined here, along with its usage. This might cause issues later when we try to update the button component based on @panchovm's plans.
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
Related to #1433 by @panchovm
Description
This PR adds a new content switcher to the homepage: modal on small screens, and popover starting from
lg.Edit: the tests for open content switcher were added for the 'old' (from
md) and 'new' homepage. The popover has an extra inline-end padding in the snapshots. It is not present on Mac (I tested on Firefox, Chrome and Safari), and I don't have a Linux machine to figure it out. I suspect that it's caused by the fix in #870, but I'm not sure.Testing Instructions
Turn on the
new_headerflag, or go to 'localhost:8443/?ff_new_header=on'. Check that the content switcher works on the homepage.Checklist
Update index.md).main) ora parent feature branch.
errors.
Developer Certificate of Origin
Developer Certificate of Origin