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

[Navigation link block][Try] Show a URL next to the link icon #23023

Closed
wants to merge 6 commits into from

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Jun 9, 2020

Description

As in the title. Solves #22836.

Before:

Zrzut ekranu 2020-06-9 o 13 28 21

After:

2020-06-24 15-45-02 2020-06-24 15_45_52

How has this been tested?

  1. Enable the navigation experiment in Gutenberg > Experiments
  2. Go to Gutenberg > Navigation (beta)
  3. Add a menu item with no link, confirm there is no text next to the link icon in the toolbar
  4. Attach a link to that menu item, confirm there link shows up next to the link icon in the toolbar

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.
  • 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.

@adamziel adamziel requested review from ajitbohra and Soean as code owners June 9, 2020 11:30
@adamziel adamziel self-assigned this Jun 9, 2020
@adamziel adamziel changed the title [Navigation link block] Show a URL next to the link icon [Navigation link block][Try] Show a URL next to the link icon Jun 9, 2020
@github-actions
Copy link

github-actions bot commented Jun 9, 2020

Size Change: +180 B (0%)

Total Size: 1.14 MB

Filename Size Change
build/annotations/index.js 3.67 kB -1 B
build/block-editor/index.js 115 kB +277 B (0%)
build/block-editor/style-rtl.css 10.8 kB +18 B (0%)
build/block-editor/style.css 10.8 kB +19 B (0%)
build/block-library/editor-rtl.css 7.73 kB +188 B (2%)
build/block-library/editor.css 7.73 kB +191 B (2%)
build/block-library/index.js 129 kB -511 B (0%)
build/components/index.js 199 kB +1 B
build/edit-post/index.js 304 kB -1 B
build/editor/index.js 45 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/api-fetch/index.js 3.39 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 7.67 kB 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-library/style-rtl.css 7.75 kB 0 B
build/block-library/style.css 7.76 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 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 48.2 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.56 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.46 kB 0 B
build/date/index.js 5.38 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.23 kB 0 B
build/edit-navigation/index.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/style-rtl.css 5.59 kB 0 B
build/edit-post/style.css 5.58 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/index.js 9.35 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 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.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 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 709 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 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 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.4 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.71 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@shaunandrews
Copy link
Contributor

Took this for a spin and have a few comments:

image

  • With the URL here, I wonder if we can remove the link icon altogether like I did above? We'd still need it to show where the block doesn't have a URL, but otherwise I don't think its doing much other than taking up space.
  • Can we remove the siteurl when linking to a post/page on the current site?
  • For longer URL's I imagine we'll need to truncate, similar to how the link UI truncates the URL.
  • This brings up a lot of questions around the connection between this button in the toolbar, and the Link UI. Part of me wonders if I should be able to edit the URL directly within the Toolbar and not need to open the Link UI.

@draganescu
Copy link
Contributor

@shaunandrews do you think maybe removing the icon makes the link too much a label, enough for the user to ignore it when they want to change the link?

@adamziel
Copy link
Contributor Author

@shaunandrews I updated this PR to reflect your suggestions. I wasn't entirely sure about the icon part so at the moment it's in a sort of intermediary state where the icon is there for new links:

Zrzut ekranu 2020-06-12 o 17 29 27

But as soon as you assign a URL, it's replaced by that URL:

Zrzut ekranu 2020-06-12 o 17 27 35

What do you think would be a proper way to address it?

This brings up a lot of questions around the connection between this button in the toolbar, and the Link UI. Part of me wonders if I should be able to edit the URL directly within the Toolbar and not need to open the Link UI.

I'm totally up for exploring that - a mockup would be more than enough to get me started. It seems like figuring out the icon would be the key here - clicking on an icon just to have it replaced by an input would be weird.

@adamziel
Copy link
Contributor Author

A lot more work is required, but an initial attempt is functional:

Zrzut ekranu 2020-06-23 o 14 04 36

@shaunandrews
Copy link
Contributor

Taking a closer look at the design for this over in #23375.

@adamziel
Copy link
Contributor Author

I took the latest design from #23375 and got an early prototype of interactions working:

85568280-d61b7d80-b631-11ea-8986-fc8842862970

There are still some visual glitches and keyboard navigation needs a closer look, but I'd like to get some early broad strokes feedback on the direction @shaunandrews @noisysocks @talldan @draganescu.

@adamziel
Copy link
Contributor Author

I will put this PR on hold for now - the design discussion is still going

@adamziel adamziel force-pushed the try/show-link-next-to-toolbar-icon branch from 1db53af to fcd4247 Compare June 30, 2020 10:54
@adamziel adamziel force-pushed the try/show-link-next-to-toolbar-icon branch from fcd4247 to 7ab307f Compare July 8, 2020 11:37
@adamziel adamziel closed this Jul 10, 2020
@aristath aristath deleted the try/show-link-next-to-toolbar-icon branch November 10, 2020 14:34
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants