8000 Add support for menu description. by nielslange · Pull Request #1771 · woocommerce/storefront · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add support for menu description. #1771

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Mar 22, 2022
Merged

Add support for menu description. #1771

merged 5 commits into from
Mar 22, 2022

Conversation

nielslange
Copy link
Contributor
@nielslange nielslange commented Aug 26, 2021 •
edited by gigitux
Loading

Note

WordPress 3.0 introduced Custom Menus which also allows adding a menu description. This PR aims to utilize this feature, so that Storefront users can show menu descriptions.

Fixes #1595

Screenshots

Before:

#1595-before

 After:

#1595-after

How to test the changes in this Pull Request:

  1. Go to /wp-admin/nav-menus.php.
  2. Click on screen options in the upper-right corner and activate the option Description within Show advanced menu properties.
  3. Create a custom menu.
  4. Add 2-3 menu items with a description.

Changelog

Add support for menu description.

@nielslange nielslange requested a review from a team as a code owner August 26, 2021 16:04
@nielslange nielslange requested review from senadir and removed request for a team August 26, 2021 16:04
@nielslange nielslange added this to the 3.9.0 milestone Sep 7, 2021
senadir
senadir reviewed Sep 7, 2021
View reviewed changes
Copy link
Member
@senadir senadir left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the code is looking good, I'm not so sure about the implementation. Other themes seems to distinguish between the item and extra text, 2021 for example.
image.
For storefront, it seems to ruin base line balance, and it's hard to distinguish between the description and the actual link (from link color, to text size and placement).
image

Can we run this by someone else first?

@nielslange
Copy link
Contributor Author

@senadir: Thanks for your initial review.

@vivialice: Would you have the capacity to spin up a design for showing the menu descriptions when using the Storefront theme? In #1771 (comment), you can see how the menu descriptions are looking currently.

@tjcafferkey tjcafferkey added the needs design The issue requires design input/work from a designer. label Sep 27, 2021
@tjcafferkey tjcafferkey removed this from the 3.9.0 milestone Sep 27, 2021
@tomasztunik tomasztunik requested a review from a team as a code owner March 21, 2022 15:24
@tomasztunik tomasztunik requested review from gigitux and removed request for a team March 21, 2022 15:24
@tomasztunik
Copy link
Contributor

Made so the items align like @senadir mentioned it is on 2021 (1.). Also updated styles to follow typographical scale used in the theme for secondary items like with cart counter (2.)
Screenshot 2022-03-21 at 19 38 50

Would be great to wrap this up but got a question - do you think adding vertical-align: top to allow this alignment as default could potentially affect child-themes or usage in the wild? In theory if people had a lot of variable length menu item names that break into multiline this could slightly alter default behaviour where it would align to the top like visible now rather than to the bottom like it was on @senadir's screenshot.

@nielslange
Copy link
Contributor Author

@tomasztunik Thanks for following up on this issue. Looking at your screenshot, I wonder if the color contrast of the menu description is sufficient, given that WordPress aims to comply to the WCAG 2.0.

@tomasztunik
Make nav menu description and cart icon product count text contrast W…
0698c8e 
…CAG 2.0 AA compliant

contrast was ~2.5, target is 4.5+
@tomasztunik
Copy link
Contributor

Bumped the contrast on description and cart count to have 4.5x contrast in the default state.

Screenshot 2022-03-21 at 21 31 12

@tomasztunik tomasztunik added this to the 4.1.0 milestone Mar 22, 2022
@tomasztunik tomasztunik merged commit 2658378 into trunk Mar 22, 2022
@tomasztunik tomasztunik deleted the add/1595-support-for-menu-description branch March 22, 2022 12:44
@gigitux
Copy link
Contributor
gigitux commented Apr 4, 2022

Hi!
I'm not sure that we can release this PR. I noticed that we have the same problem with the alignment when I display the menu as Secondary Menu.

image

My suggestion is to revert the PR and fix this problem too. What do you think?

@nielslange
Copy link
Contributor Author

My suggestion is to revert the PR and fix this problem too. What do you think?

@gigitux First of all, nice catch. I totally missed the submenu descriptions. I wonder if reverting is less effort, though, than spinning up an additional PR to cover the submenu as well. You know what, I'll quickly spin up another PR. You can then release Storefront either with both PRs, if mine gets reviewed fast enough. Or you revert this PR and the next porter releases these features next week.

@nielslange nielslange mentioned this pull request Apr 4, 2022
Open
gigitux added a commit that referenced this pull request Apr 4, 2022
@gigitux
Revert "Add support for menu description. (#1771)"
7257f8f 
This reverts commit 2658378.
@gigitux gigitux restored the add/1595-support-for-menu-description branch April 4, 2022 12:12
@gigitux gigitux mentioned this pull request Apr 4, 2022
Merged
gigitux added a commit that referenced this pull request Apr 4, 2022
@gigitux
Revert "Add support for menu description. (#1771)" (#1944)
64f5950 
This reverts commit 2658378.
gigitux added a commit that referenced this pull request Apr 4, 2022
@gigitux
Revert "Add support for menu description. (#1771)" (#1944)
8ce6f51 
This reverts commit 2658378.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
73A5
Reviewers

@senadir senadir senadir left review comments

@gigitux gigitux Awaiting requested review from gigitux gigitux was automatically assigned from woocommerce/woo-fse

Assignees
No one assigned
Labels
needs design The issue requires design input/work from a designer. type: compatibility
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.

Support for menu description.
5 participants
@nielslange @tomasztunik @gigitux @senadir @tjcafferkey
0