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.

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

Allow wrapping for Navigation block links #21632

Merged
merged 4 commits into from
Apr 17, 2020
Merged

Allow wrapping for Navigation block links #21632

merged 4 commits into from
Apr 17, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Apr 16, 2020

Description

Fixes #18337.

Wraps text within a navigation link.

Links already had a width: max-content; rule, which prevented wrapping, so I had to remove this.

I also noticed when wrapping occurs, the submenu item indicator is positioned in an unusual way (it was always positioned on the right side of the block), so this PR also fixes that by changing the location of the indicator in the DOM. The indicator is now brought inline with the text.

There's an issue in IE11 where the indicator wraps to the next line (see screenshot). I haven't been able to work out how to fix that. Advice/commits are welcome. I don't think it's any worse than it was and it only occurs when the line wraps—here's a non-wrapping line:
Screenshot 2020-04-16 at 11 34 48 am

How has this been tested?

  1. Add a navigation block
  2. Add a navigation link and type in some very long text (preferably a very long word)
  3. See the wrapping

Screenshots

Before (Firefox)

Screenshot 2020-04-16 at 11 04 49 am

After (Firefox)

Screenshot 2020-04-16 at 10 59 16 am

Before (IE 11)

Screenshot 2020-04-16 at 11 28 16 am

After (IE 11)

Screenshot 2020-04-16 at 11 26 01 am

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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.

@talldan talldan added [Type] Bug An existing feature does not function as intended [Block] Navigation Affects the Navigation Block labels Apr 16, 2020
@talldan talldan self-assigned this Apr 16, 2020
@talldan talldan changed the title Allow wrapping for links Allow wrapping for Navigation block links Apr 16, 2020
@github-actions
Copy link

github-actions bot commented Apr 16, 2020 •
edited
Loading

Size Change: +249 B (0%)

Total Size: 840 kB

Filename Size Change
build/block-library/style-rtl.css 7.14 kB +1 B
build/block-library/style.css 7.15 kB +1 B
build/components/index.js 198 kB -1 B
build/components/style-rtl.css 16.7 kB +119 B (0%)
build/components/style.css 16.7 kB +118 B (0%)
build/editor/index.js 43.3 kB +5 B (0%)
build/url/index.js 4.02 kB +6 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.01 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.1 kB 0 B
build/block-library/editor.css 7.1 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.1 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.1 kB 0 B
build/edit-navigation/style-rtl.css 279 B 0 B
build/edit-navigation/style.css 280 B 0 B
build/edit-post/index.js 27.8 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.4 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.53 kB 0 B
build/edit-widgets/style-rtl.css 4.65 kB 0 B
build/edit-widgets/style.css 4.64 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/style-rtl.css 3.48 kB 0 B
build/editor/style.css 3.47 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.91 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.28 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@tellthemachines
Copy link
Contributor

I also noticed when wrapping occurs, the submenu item indicator is positioned in an unusual way (it was always positioned on the right side of the block), so this PR also fixes that by changing the location of the indicator in the DOM. The indicator is now brought inline with the text.

Erm, I moved the indicator out of the anchor tag in #21471 so that the dropdown can be opened on a touchscreen. The problem with the icon being inside the link is that tapping on it on a phone will just click through to the link, so the submenus are effectively unusable. It would probably work better to make it more visibly a button instead of appearing to be inline - @karmatosed could this be an option?

@talldan
Copy link
Contributor Author

talldan commented Apr 16, 2020 •
edited
Loading

Erm, I moved the indicator out of the anchor tag in #21471 so that the dropdown can be opened on a touchscreen. The problem with the icon being inside the link is that tapping on it on a phone will just click through to the link, so the submenus are effectively unusable. It would probably work better to make it more visibly a button instead of appearing to be inline - @karmatosed could this be an option?

Ah, ok, didn't realise that, sorry about that!

Agree it probably needs some further thought. Would be good to research what menus look like on the themes/websites that are already out there and see how they can be supported.

A few I looked at seem to either just not bother with submenus on mobile (Gumtree), or switch to a fold out menu where the submenu opener is no longer a link (Amazon, Australia Post). Some use the latter approach on both desktop and mobile (The Guardian), which is one of the reasons I think we should support menu items that don't have links.

@talldan
Copy link
Contributor Author

talldan commented Apr 16, 2020

For now I've reverted the change to the submenu icons so that this can move forwards.

tellthemachines
tellthemachines approved these changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Works well as a first iteration 🙂

@talldan talldan merged commit 10e19e0 into master Apr 17, 2020
@talldan talldan deleted the fix/nav-link-wrapping branch April 17, 2020 06:15
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 17, 2020
@dependabot dependabot bot mentioned this pull request Mar 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tellthemachines tellthemachines tellthemachines approved these changes

@draganescu draganescu Awaiting requested review from draganescu

@adamziel adamziel Awaiting requested review from adamziel

@karmatosed karmatosed Awaiting requested review from karmatosed

@noisysocks noisysocks Awaiting requested review from noisysocks

@mapk mapk Awaiting requested review from mapk

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@Soean Soean Awaiting requested review from Soean

Assignees

@talldan talldan

Labels
[Block] Navigation Affects the Navigation Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 8.0
Development

Successfully merging this pull request may close these issues.

Wrapping probably should happen for navigation
2 participants
@talldan @tellthemachines