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

Navigation: Make Navigation a static block #24371

Closed
wants to merge 1 commit into from
Closed

Navigation: Make Navigation a static block #24371

wants to merge 1 commit into from

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Aug 5, 2020 •
edited
Loading

Work in progress.

Implements part of #24364.

Makes Navigation a static block and Navigation Link a mostly static block.

Tasks remaining:

  • Fix block context not being passed to save().
  • Handle migration of older rgb* attributes.
  • Test all attributes.
  • Test migration from older blocks.

@noisysocks noisysocks added the [Block] Navigation Affects the Navigation Block label Aug 5, 2020
@github-actions
Copy link

github-actions bot commented Aug 5, 2020

Size Change: +360 B (0%)

Total Size: 1.16 MB

Filename Size Change
build/block-library/index.js 133 kB +360 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 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.97 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/index.js 125 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.59 kB 0 B
build/block-library/editor.css 7.59 kB 0 B
build/block-library/style-rtl.css 7.76 kB 0 B
build/block-library/style.css 7.77 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.3 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.68 kB 0 B
build/core-data/index.js 11.8 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.45 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 568 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/index.js 10.9 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/index.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/index.js 17 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 9.38 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/index.js 45.3 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.79 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.72 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 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 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.11 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.33 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.41 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.27 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.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@noisysocks
Copy link
Member Author

Here's a first pass at making Navigation a static block. I like the amount of red—it's a big simplification.

Unfortunately, though, there's a problem: any existing dynamic Navigation blocks that are out there will not render properly in the frontend until the user re-saves the post that the block is in. This is because we're deleting the render_callback and so WordPress won't know what to do with the <!-- wp:navigation /-->. Re-saving the post fixes it because Gutenberg will replace the <!-- wp:navigation /--> with what save() returns.

We either have to:

  1. Not bother doing this and keep everything dynamic.
  2. Keep the render_callback around for a while. (Forever?)
  3. Accept that Navigation is experimental and that sites using it may break.

Thoughts? @talldan @draganescu @adamziel

@noisysocks noisysocks mentioned this pull request Aug 5, 2020
Closed
9 tasks
@noisysocks
Copy link
Member Author

This was discussed during the Nav sync in #core where we decided:

  1. Let's keep Link dynamic.
  2. Let's keep the render_callback around for two releases with a deprecation notice.

@youknowriad
Copy link
Contributor

I thought it was important for that block to be dynamic in order to adapt to changes to the permalink structure...?

@noisysocks
Copy link
Member Author

I thought it was important for that block to be dynamic in order to adapt to changes to the permalink structure...?

Yes, we'll need to keep Link dynamic for this reason. Do you think Navigation should remain dynamic as well?

@youknowriad
Copy link
Contributor

Do you think Navigation should remain dynamic as well?

What do you mean by Navigation here? I personally don't know enough here to share an informed opinion. I know initially, @mtias was in favor of a fully dynamic block.

@adamziel
Copy link
Contributor

adamziel commented Aug 6, 2020

@youknowriad Rob meant we could use a combination of static (navigation) and dynamic blocks (link) to have both:

  • Auto-updating links when dynamic blocks are available
  • A decent fallback in all other cases (like using a classic editor)

@noisysocks
Copy link
Member Author

noisysocks commented Aug 7, 2020 •
edited
Loading

I figured that if a block can be static then it should be static. Since Navigation merely renders a container <nav><ul><InnerBlocks /></ul></nav>, it can be static.

Happy to keep both Navigation and Link dynamic if we think that's better. The argument about compatibility with the Classic Editor holds less water when we're talking about Navigation because it's not common that you would insert a Navigation block into a post or page.

What do you think?

@youknowriad
Copy link
Contributor

It makes sense for me 👍 I just wanted to make sure we were not regressing on the dynamic behaviors.

@mtias
Copy link
Member

mtias commented Aug 25, 2020

Not sure I see much value in keeping an empty container static when it doesn't really provide useful semantics. I always thought a potential static fallback for a navigation block should instead be a semantic link to a site's sitemap (new feature in 5.5!)or something to that effect.

@noisysocks
Copy link
Member Author

At any rate, this PR is stale! Closing it for now.

@noisysocks noisysocks closed this Aug 26, 2020
@noisysocks noisysocks deleted the update/make-navigation-block-static branch August 26, 2020 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
[Block] Navigation Affects the Navigation Block
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@noisysocks @youknowriad @adamziel @mtias