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

Extract navigation link rendering code from the navigation block #21075

Merged
merged 10 commits into from
Jul 3, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Mar 23, 2020

Description

Fixes #20178. Makes the Navigation Link block dynamic and relocates much of the rendering to this block.

For this PR, I've had to change how some features work:

  • The submenu icons are now hidden using a display: none; style when turned off. Previously they weren't rendered at all.
  • Font size and text color styles when viewing a post continue to work through CSS inheritance, the navigation link doesn't have any specific classnames or inline styles set to determine those colors.
  • Background color no longer works when viewing a post, for the same reason as above, there are no specific classnames or inline styles set on the navigation link It's harder for the background colors to be inherited, as this isn't the natural behavior for most elements in CSS.

All of the above points could be resolved/improved by pushing forward the proposal in #19685.

Server side rendered blocks could declare a context for child blocks, which would allow passing attributes to child Navigation Link blocks, and the rendering could work much as it did before.

How has this been tested?

  1. Add a navigation block
  2. Build out navigation block with lots of nested navigation links.
  3. Preview the post.
  4. Observe that the navigation still renders

Screenshots

Types of changes

Code quality improvement

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] Task Issues or PRs that have been broken down into an individual action to take [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality [Block] Navigation Affects the Navigation Block labels Mar 23, 2020
@talldan talldan self-assigned this Mar 23, 2020
@github-actions
Copy link

github-actions bot commented Mar 23, 2020

Size Change: +182 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-library/index.js 130 kB +91 B (0%)
build/block-library/style-rtl.css 7.79 kB -1 B
build/block-library/style.css 7.79 kB -1 B
build/edit-navigation/index.js 9.97 kB +93 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 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.16 kB 0 B
build/block-directory/style-rtl.css 952 B 0 B
build/block-directory/style.css 951 B 0 B
build/block-editor/index.js 109 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.63 kB 0 B
build/block-library/editor.css 7.64 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 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/index.js 198 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.65 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 569 B 0 B
build/dom/index.js 3.19 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/index.js 304 kB 0 B
build/edit-post/style-rtl.css 5.57 kB 0 B
build/edit-post/style.css 5.56 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.02 kB 0 B
build/edit-site/style.css 3.02 kB 0 B
build/edit-widgets/index.js 9.32 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 44.8 kB 0 B
build/editor/style-rtl.css 3.82 kB 0 B
build/editor/style.css 3.82 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.73 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 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.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.3 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.5 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.69 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.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@draganescu
Copy link
Contributor

draganescu commented Mar 24, 2020

This is gonna sound a bit weird, but I forgot why is navigation-link a block? The same goes for column. These feel more like components inside their parent blocks since once can't use them by themselves anyway.

Also this Server Side Rendering Parent Attributes Context should help with colors and fonts set on the Navigation block, right?

@talldan
Copy link
Contributor Author

talldan commented Mar 25, 2020

@draganescu Yes, that looks like it'd be helpful.

This is gonna sound a bit weird, but I forgot why is navigation-link a block? The same goes for column. These feel more like components inside their parent blocks since once can't use them by themselves anyway.

For column, that's needed because columns are multi dimenional, but inner blocks only has a single dimension. Using two nested inner blocks is/was the solution for that.

For navigation link, I suppose that could be implemented without using inner blocks, but that seems like it's going against the grain. The block would have to re-implement appending, moving, duplicating etc.

@draganescu
Copy link
Contributor

Indeed, we need column and nav-item to be able to use the innerBlocks api but they look a lot like private components of the "real" blocks, columns and navigation.

Perhaps a more complex innerBlocks API could allow us to use arbitrary components too (since they're all react components) not only blocks and hence not pollute the library with private components disguised as blocks.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

This is looking good! I left some questions and relatively minor comments.

This breaking the ability to set a background colour is a bummer. Is #19572 close to being merged? It's probably worth waiting for that or to help push it forward.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

I tested this and it behaves very nicely. My opinion is that we should merge this asap as it brings refactoring to the whole block. Considering all the work going on with this block maintaining the PR will be very hard.

We can fix the background color for nested menu items later.

Also most of @noisysocks 's comments to navigation-item/index.php also apply to navigation/index.php.

#19685 will bring the needed updates and then many of the issues still present here will be fixed properly, but the base will be covered by how the Navigation block works.

@talldan
Copy link
Contributor Author

talldan commented Apr 7, 2020

@noisysocks I've addressed the code review comments. I've avoided making code quality improvements though, as most of the code was only moved and the issues already exist in master in the navigation block.

I'm happy to bring those in a follow-up PR though, I think that'd make the git history cleaner to follow and would avoid making this PR more complicated than it already is.

I'm with Andrei in that I think it'd be good to merge sooner, but I do also feel like #19685 might not be straightforward.

@noisysocks
Copy link
Member

I'm concerned that we don't have an approach in mind for how to fix background colours that doesn't involve #19685 which might not eventuate into anything. Any ideas?

@noisysocks
Copy link
Member

Oh! Just saw that @aduth has taken a new approach to implementing block context in #21467. The approach looks really solid to me. Let's help get it merged.

@adamziel
Copy link
Contributor

adamziel commented Jul 2, 2020

Is this one is ready for testing/review after the recent changes?

talldan and others added 8 commits July 3, 2020 12:03
Move code around with TODOs where needed

Migrate away from block.json

Filter out empty blocks

Minor tweaks

Add navigation link to core dynamic block types

Temporarily comment out code in nav-link block that is coupled to nav block

Minor fixes

Actually render the inner blocks

This is probably fine

Render nav link inner blocks in a <ul>

Make the Show Submenu Icon feature work using CSS

Ensure default attributes work correctly on nav and nav link blocks

Fix has-submenu-icon classname application in render function

Make color styles inherit

Move navigation link styles to the navigation link style.scss

Fix & being used in base level style

Remove TODO that has now been TODONE

Fix PHP linting

Update navigation link snapshot test

Fix PHP doc block comments

Co-Authored-By: Robert Anderson <robert@noisysocks.com>
@noisysocks
Copy link
Member

Is this one is ready for testing/review after the recent changes?

It is now! ✌️ I've rebased it, addressed remaining comments, and did some side-by-side testing and fixing against master.

@talldan
Copy link
Contributor Author

talldan commented Jul 3, 2020

👍 I gave this a good test, and as far as I can tell all the features of the block are still working.

This change improves the code quite a bit. Removing the render block filters and bringing the rendering under the responsibility of the navigation link will be important for the code quality as the blocks continue to be developed.

I'm unable to approve as I'm the PR author, but I think given we've both peer reviewed one another's changes we should merge this.

@noisysocks
Copy link
Member

I'm unable to approve as I'm the PR author, but I think given we've both peer reviewed one another's changes we should merge this.

Asynchronous pair programming FTW! 🤩

@noisysocks noisysocks merged commit 55d7f85 into master Jul 3, 2020
@noisysocks noisysocks deleted the try/dynamic-nav-link branch July 3, 2020 04:34
@github-actions github-actions bot added this to the Gutenberg 8.5 milestone Jul 3, 2020
@talldan talldan removed [Type] Task Issues or PRs that have been broken down into an individual action to take [Status] In Progress Tracking issues with work in progress labels Jul 3, 2020
@talldan talldan changed the title Try extracting Navigation Link rendering from the Navigation Block Extract navigation link rendering from the navigation block, use block context, and remove custom render filters Jul 3, 2020
@talldan talldan changed the title Extract navigation link rendering from the navigation block, use block context, and remove custom render filters Extract navigation link rendering code from the navigation block Jul 3, 2020
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 [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Navigation] Consider making Navigation Link a dynamic block
5 participants