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

Enable open on click for Page List inside Navigation. #34675

Merged
merged 48 commits into from
Sep 15, 2021

Conversation

tellthemachines
Copy link
Contributor

@tellthemachines tellthemachines commented Sep 9, 2021

Description

Depends on #33775 and is currently pointed to that branch.

Enables open on click functionality for Page List blocks nested inside Navigation, whenever the open on click option is set in Navigation.

Consolidates the front end script from Submenu with the Navigation one, so that it can be used by all child blocks of Navigation.

When open on click is set, in the editor, submenus really open on focus. Because the block is server-rendered, it's tricky to add click events in the editor, and I think this is a good compromise. For mouse/touch users, it will work the same, whereas for keyboard users it shouldn't be a major issue because Select mode already allows skipping nested blocks when tabbing through items.

How has this been tested?

Add a Navigation block with a Page List inside it, and enable "open on click" in the Navigation block sidebar. Verify that Page List submenus open on click instead of hover/focus.

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@jasmussen
Copy link
Contributor

Nice, this feels close to me! When the click option is toggled on, it appears to behave well for me in editor and frontend:

Screenshot 2021-09-10 at 10 00 23

We should probably use the cursor: pointer; even on navigation items that open on click using a button — it feels a bit broken without it.

When I have click toggled off, I see what looks like a button around the dropdown chevron:

Screenshot 2021-09-10 at 10 00 27

And something a bit confusing appeared to happen to me on the frontend when previewing. When I first tested, and created a hover menu, I got this to show:

Screenshot 2021-09-10 at 10 00 43

In this view, outside of the styling issue, focus also appeared to behave as intended.

Then I enabled the click option and saved and previewed, but it required a few reloads for the effect to take. Once it finally stuck, I was unable to go back to hover-only. Same with hiding the chevron — this did not appear to save on the frontend.

I might be experiencing a caching issue of sorts. But in any case, these steps failed for me:

  • Create a nav block with a page list, then toggle click, save, preview, reload.
  • Untoggle click, save, preview, and menus still open on click.
  • Untoggle chevrons, save, preview, chevrons still show up.

Any idea what's up here?

Copy link
Member

@vcanales vcanales left a comment

Choose a reason for hiding this comment

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

Consolidates the front end script from Submenu with the Navigation one, so that it can be used by all child blocks of Navigation.

Thanks for this! It looks good 🎉

I noticed a few issues (screen caps from Firefox)

  1. In the editor, opening a submenu with multiple children opens all of them, and I can't seem to close the children by themselves.
    Kapture 2021-09-10 at 17 36 32
  2. In the frontend, clicking on a sub-submenu closes the parent.
  3. At least on TT1, buttons are being styled in a few different ways depending on the settings; showing you what I see in a table below:
Open on click enabled Open on click disabled
Frontend Kapture 2021-09-10 at 17 47 09 Kapture 2021-09-10 at 17 45 45
Editor Screen Shot 2021-09-10 at 17 43 18 Screen Shot 2021-09-10 at 17 43 53

Base automatically changed from add/dropdown-block to trunk September 13, 2021 01:18
@tellthemachines
Copy link
Contributor Author

Thanks for reviewing and testing, folks!

All the issues should now be fixed:

  • The button toggles and dropdowns were missing some styles, that's why the indicators were looking weird and the submenus were opening all at once.
  • The parent closing on clicking a sub-submenu was due to a bug in the front end script.
  • The front end not updating due to changing from open on click to hover and toggling submenu icons was due to the context not being properly saved as attributes in the block edit.

Let me know if I've missed anything 😅

Copy link
Member

@vcanales vcanales left a comment

Choose a reason for hiding this comment

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

Looking good! I'm only noticing one minor remaining issue, again on TT1; buttons for links that contain submenus look differently in the editor and the frontend:

Editor Frontend
image image

In trunk they show up as they do in the editor; ie. no background.

@tellthemachines
Copy link
Contributor Author

buttons for links that contain submenus look differently in the editor and the frontend:

That is when Navigation is set to open on click, right? The problem is Twenty Twenty One has some super specific button styling, but there's not much we can do about that because we want to keep editor style specificity as low as possible. It will need to be fixed in the theme sooner or later.

Copy link
Member

@vcanales vcanales left a comment

Choose a reason for hiding this comment

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

That is when Navigation is set to open on click, right?

Indeed, it happens only when open on click is on.

there's not much we can do about that because we want to keep editor style specificity as low as possible. It will need to be fixed in the theme sooner or later.

With this in mind, I think this PR good to go :)

@tellthemachines tellthemachines merged commit d065e74 into trunk Sep 15, 2021
@tellthemachines tellthemachines deleted the add/page-list-click-open branch September 15, 2021 00:06
@github-actions github-actions bot added this to the Gutenberg 11.6 milestone Sep 15, 2021
fullofcaffeine added a commit that referenced this pull request Sep 16, 2021
* trunk: (74 commits)
  Update docs for ClipboardButton component (#34711)
  Post Title Block: add typography formatting options (#31623)
  Bump plugin version to 11.5.0
  Navigation Screen: Adjust header toolbar icon styles (#34833)
  Fix the parent menu item field in REST API responses (#34835)
  Rewrite FocusableIframe as hook API (#26753)
  Create Block: Remove wp-cli callout since not recommended and outdated (#34821)
  Global Styles: Fix dimensions panel default controls display (#34828)
  [RNMobile][Embed block] Enable embed preview for Instagram and Vimeo providers (#34563)
  Increase Link UI search results to 10 on Nav Editor screen (#34808)
  Prevent welcome guide overflow x scroll (#34713)
  Enable open on click for Page List inside Navigation. (#34675)
  [RNMobile] [Embed block] -  Unavailable preview fallback bottom sheet title update  (#34674)
  Add missing field _invalid in menu item REST API (#34670)
  Fix Dropdown/DropdownMenu toggle closing in all UAs (#31170)
  Navigation submenu block: replace global shortcut event handlers with local ones (#34812)
  Navigation Screen: Consolidate menu name and switcher (#34786)
  Remove parent and position validation from menu item REST API endpoint (#34672)
  Clean theme data when switching themes in the customizer (#34540)
  Components: add reset timeout to ColorPicker's copy functionality. (#34601)
  ...

// Styles for button toggles.
button.wp-block-navigation__submenu-icon.wp-block-navigation__submenu-icon.wp-block-navigation__submenu-icon.wp-block-navigation__submenu-icon {
padding: $grid-unit $grid-unit-20;
Copy link
Contributor

Choose a reason for hiding this comment

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

In my testing, this specificity wasn't necessary, and the added padding makes the chevron look very different from editor and frontend. I have a followup in #35030.

@mcsf mcsf added [Block] Navigation Affects the Navigation Block [Block] Page List Affects the Page List Block [Type] Enhancement A suggestion for improvement. labels Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Block] Page List Affects the Page List Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants