Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Styles: Add "Buttons" block style for navigation block #127

Merged
merged 3 commits into from Mar 22, 2024

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Mar 12, 2024

See WordPress/pattern-directory#635 — The category navigation uses a row of links, so a navigation block is appropriate for the markup, but the individual items use "button" styles for active (and presumably hover/focus, though not mocked up there).

I've added this to the parent theme for two reasons: I thought this pattern might pop up again, maybe in plugins, and I wanted to easily share the block mixin code.

Screenshots

In the first set of screenshots, "Featured" is in the hover state, and "Posts" is currently selected. In the second, we're on the homepage so no current selection, and "Featured" is in the focus state.

Before After
before after
before after

cc @WordPress/meta-design Do those interaction states work?

How to test the changes in this Pull Request:

  1. Add a navigation block (in a post, page, or the site editor)
  2. Switch to the styles tab & select "Buttons"
  3. There won't be much of a change in the editor, but on the frontend the navigation links should behave more like buttons: there is padding around them, the background changes on hover, and the currently-selected item should have a black background.
  4. If you're testing in a post/page, add the current page to the menu to check the current selected state.

@jasmussen
Copy link

Looks good. I'll defer to @fcoveram who may have more precise metrics, but from my angle the buttons look a bit bigger than they need to be (height/padding). This may just be an optical illusion so feel free to ignore, but from the original designs, it's 40px buttons with 12px padding left and right. I'd also use the lightest gray we have for the hover style.

@fcoveram
Copy link

Here is the link to the button component's design spec used for each tab, in case the design documentation is not fully clear.

Do those interaction states work?

Yes. The only change is in hover, where background color changes to Light grey 2.

Based on the screenshots, I would say that All tab is not necessary if clicking on it takes you to see the same in a different page. Am I correct with the interaction flow?

@ryelle
Copy link
Contributor Author

ryelle commented Mar 18, 2024

Here is the link to the button component's design spec used for each tab, in case the design documentation is not fully clear.

It's not clear… which button-grouping are you referring to? The link landed me on the whole zoomed out artboard.

Based on the screenshots, I would say that All tab is not necessary if clicking on it takes you to see the same in a different page.

In this case, "All" would take you back to the main (unfiltered) Archive page. Which, yes, we probably don't need since the "All patterns" link in the breadcrumbs is right there.

Screenshot 2024-03-18 at 10 26 18 AM

@ryelle
Copy link
Contributor Author

ryelle commented Mar 20, 2024

Updated the hover color:

Screenshot 2024-03-20 at 12 47 18 PM

@fcoveram
Copy link

It's not clear… which button-grouping are you referring to? The link landed me on the whole zoomed out artboard.

Oh sorry for the useless deep link. The component is named Button / Small, here is a screenshot from the Figma file.

Screenshot of Button Small component from WordPress.org Design Library

@ryelle
Copy link
Contributor Author

ryelle commented Mar 20, 2024

@fcoveram Do I need to update anything for this PR?

@fcoveram
Copy link

Based on the image attached, and if styles per state are according to the library, then everything is fine.

I struggled with technical issues when trying to run the PR in my local, and asking for help with that might take more time than needed to give a proper answer.

@ryelle
Copy link
Contributor Author

ryelle commented Mar 22, 2024

Okay, I'll merge this & you can check it with the theme preview.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants