Skip to content

Try/sub nav editing appender#22165

Merged
adamziel merged 3 commits into
try/sub-nav-editingfrom
try/sub-nav-editing-appender
May 11, 2020
Merged

Try/sub nav editing appender#22165
adamziel merged 3 commits into
try/sub-nav-editingfrom
try/sub-nav-editing-appender

Conversation

@adamziel

@adamziel adamziel commented May 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This is a follow-up on #22107 to remove the appender from non-active navigation blocks:

2020-05-11 14-53-19 2020-05-11 14_54_14

I extracted this into a separate PR because it broke multiple tests and I didn't want it to block the other changes.

How has this been tested?

  1. Add a new navigation block with a few levels of nesting
  2. Navigate between submenus and confirm the appender is only visible when on the active menu

Types of changes

Breaking change (fix or feature that would cause existing functionality to not work as expected)

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 added [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Block] Navigation Affects the Navigation Block labels May 7, 2020
@adamziel adamziel self-assigned this May 7, 2020
@adamziel adamziel changed the base branch from master to try/sub-nav-editing May 7, 2020 10:23
@adamziel adamziel requested review from draganescu and karmatosed May 7, 2020 10:24
@adamziel adamziel marked this pull request as ready for review May 7, 2020 10:24
@adamziel adamziel mentioned this pull request May 7, 2020
Closed
@github-actions

github-actions Bot commented May 7, 2020 •
edited
Loading

Copy link
Copy Markdown

Size Change: +154 B (0%)

Total Size: 822 kB

Filename Size Change
build/block-library/index.js 116 kB +156 B (0%)
build/block-library/style-rtl.css 7.34 kB -1 B
build/block-library/style.css 7.34 kB -1 B
ℹ️ 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.08 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.6 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 101 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.12 kB 0 B
build/block-library/editor.css 7.12 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 48.1 kB 0 B
build/components/index.js 179 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 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.44 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 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 4.07 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 28.1 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12.3 kB 0 B
build/edit-site/style-rtl.css 5.19 kB 0 B
build/edit-site/style.css 5.2 kB 0 B
build/edit-widgets/index.js 8.37 kB 0 B
build/edit-widgets/style-rtl.css 4.68 kB 0 B
build/edit-widgets/style.css 4.68 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.63 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.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 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.29 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.56 kB 0 B
build/primitives/index.js 1.5 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 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/url/index.js 4.02 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.18 kB 0 B

compressed-size-action

@adamziel adamziel mentioned this pull request May 7, 2020
Merged
6 tasks
@draganescu

draganescu commented May 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Yes this is great! As my comment explained it's a bit weird to be suddenly without the appender and be forced to use the toolbar. This way it is at least predictable when the appender appears.

@karmatosed

karmatosed commented May 7, 2020

Copy link
Copy Markdown
Member

I like it. Just going to loop in @jasmussen to be sure we're keeping to patterns that are expected.

karmatosed
karmatosed approved these changes May 7, 2020
View reviewed changes

@karmatosed karmatosed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From a design perspective going to approve, thanks.

@jasmussen

jasmussen commented May 8, 2020

Copy link
Copy Markdown
Contributor

Yep, seems like a good small win!

@talldan

talldan commented May 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

This felt natural at first, but then I noticed some situations where there's no appender, typically when there are navigation links selected and they have no children:

Nested block with no children

Screenshot 2020-05-08 at 2 32 15 pm

Root level block with no children

Screenshot 2020-05-08 at 2 32 26 pm

I think this may end up being confusing to some users who just want to add another sibling block, but I'm not sure what the best solution would be. Maybe the appender at the same level as the block should be visible if the selected block has no children?

@jasmussen

jasmussen commented May 8, 2020

Copy link
Copy Markdown
Contributor

I'm having some issues with my environment so I can't test this — but is it related to what has focus, @talldan? Any menu item can have both item focus and text editing focus.

@talldan

talldan commented May 8, 2020

Copy link
Copy Markdown
Contributor

@jasmussen It seems to happen with either kind of focus.

@adamziel

adamziel commented May 11, 2020

Copy link
Copy Markdown
Contributor Author

@talldan this should be fixed now, would you confirm please? Also CC @karmatosed

2020-05-11 14-53-19 2020-05-11 14_54_14

@karmatosed

karmatosed commented May 11, 2020

Copy link
Copy Markdown
Member

Looking at the gif, that's awesome! Thanks for working on this as I really believe only having one appender makes a lot more sense.

@draganescu

draganescu commented May 11, 2020

Copy link
Copy Markdown
Contributor

Finally this feels natural :D

@adamziel adamziel merged commit 91cd6d0 into try/sub-nav-editing May 11, 2020
@adamziel adamziel deleted the try/sub-nav-editing-appender branch May 11, 2020 14:40
@noisysocks

noisysocks commented May 13, 2020

Copy link
Copy Markdown
Member

@adamziel: Did you mean to merge this? I'm not seeing these changes in master because this was merged into try/sub-nav-editing.

@adamziel adamziel mentioned this pull request May 13, 2020
Merged
6 tasks
@adamziel

adamziel commented May 13, 2020

Copy link
Copy Markdown
Contributor Author

@noisysocks ah good spot, thank you! I just spinned a new PR here: #22311

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@karmatosed karmatosed karmatosed approved these changes
@draganescu draganescu Awaiting requested review from draganescu
@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner
@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta jorgefilipecosta is a code owner
@Soean Soean Awaiting requested review from Soean Soean is a code owner
@talldan talldan Awaiting requested review from talldan talldan is a code owner

Assignees

@adamziel adamziel

Labels

[Block] Navigation Affects the Navigation Block [Feature] List View Menu item in the top toolbar to select blocks from a list of links.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@adamziel @draganescu @karmatosed @jasmussen @talldan @noisysocks