-
Notifications
You must be signed in to change notification settings - Fork 63
Add homepage content switcher for mobile screens #716
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great ✨ I just noticed the following:
- Margin-right of content icon is
8px
, it should be4px
. - Padding-right of content button is
8px
, it should be12px
to look balanced. Hover
should look likeActive
state. Sorry for not adding it to Design Libary, I will do it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments:
I am not seeing the hover style. When pointer is over the button, this does not change its style.
The Active Focus
style needs to add a 1px
white border inside. I added the component into the Design Library.
I just checked and I am seeing the last commit. The button does not change when pointer goes over it, just when I clicked on it gets the dark background. Here is my pointer passing over the buttons without click. And the |
@panchovm could you try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice and is better than having no content switcher, so LGTM!
I do wonder from an a11y perspective if this needs to simulate or use a radio group, since we have multiple buttons that change a single value. We can look into that in a subsequent PR.
I ran the pull and it shows |
@panchovm that's useful information, we'll wait for others to test and see if we can replicate the behavior you're seeing. Just because it works for Olga and I doesn't mean it's working. Could always be a browser or system issue. A reminder as well, the primary use case for this component is mobile, where hover states are not present. |
Definitely agree, sorry for missing this a11y point. Issue created: WordPress/openverse#627 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! The hover styles work fine for me... I wonder what's going on with @panchovm's local 🤔 It worked in Safari, Chrome and Firefox for me.
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
* main: (73 commits) Make audio/image pages without ids show a 404 (#768) Fix logo button paddings and simplify implementation (#767) Fix global audio rtl close placement (#780) Check for `null` localStorage explicitly (#763) Truncate global audio text to two lines (#773) New image details page (#682) Switch to path-based i18n routing (#701) Enable source maps in production (#755) Remove Jamendo and Wikimedia Commons from audio meta sources (#747) Use jed1x json format to correctly handle pluralization (#753) Fix logo color on error page layout (#752) Add homepage content switcher for mobile screens (#716) Add inline-start border to filters on desktop (#748) Fix header items not fitting in (#718) Expose `close` to popover content via slot props (#736) Truncate row layout audio titles (#735) Stop blocking on analytics requests (#715) Style links globally (#727) Refactor the usage of i18n result count computation (#707) Use `VPopover` for the content report form (#719) ...
Fixes
Fixes #709 by @obulat
Description
This PR adds the content type switcher buttons to the homepage that are used on mobile screen. They are hidden on larger screen using tailwind utilities.
@panchovm , I don't know what the
hover
styles should be for the buttons.Testing Instructions
Run the app, open the homepage on a narrow screen, and try changing the content types, and searching for different content types.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin