List View: Simplify the BlockNavigation component #31290
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Copons
added
[Type] Code Quality
|
Size Change: -97 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Copons
requested review from
vindl,
jeyip,
mattwiebe,
david-szabo97,
Addison-Stavlo,
youknowriad,
talldan,
adamziel and
jorgefilipecosta
packages/block-editor/src/components/block-navigation/use-block-navigation-client-ids.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-navigation/use-block-navigation-client-ids.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-navigation/use-block-navigation-client-ids.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-navigation/use-block-navigation-client-ids.js
Show resolved
Hide resolved
| import { isClientIdSelected } from './utils'; | ||
| import { store as blockEditorStore } from '../../store'; | ||
|
|
||
| const noop = () => {}; | ||
|
|
||
| export default function BlockNavigation( { |
There was a problem hiding this comment.
It seems we don't really use this component, as we are using the Tree component for the list views. It also seems this is not exported either.
Im only seeing this used in /block-navigation/dropdown.js ? Maybe it makes sense to get rid of this component altogether and just update dropdown.js to use Tree appropriately?
It also seems odd for this file to be named index.js. It doesn't look like we export this, im only seeing exports for dropdown, block-slot, editor, and tree specifically.
There was a problem hiding this comment.
It's definitely odd and, to be entirely honest, it has always seemed odd to me.
It's kinda clear to me that Tree should be the main component, and index.js should be integrated into Dropdown.
Although, Tree and Dropdown are exported by the package (even if __experimental), so there is a chance of accidentally breaking something.
There was a problem hiding this comment.
Im only seeing this used in /block-navigation/dropdown.js ? Maybe it makes sense to get rid of this component altogether and just update dropdown.js to use Tree appropriately?
I agree with this and and since BlockNavigation is not exported and only BlockNavigationDropdown is, the implementation details are irrelevant as long as it keeps working as before.
Also changes at Tree should be okay since it's __experimental.
There was a problem hiding this comment.
I've created a separate PR to address the restructuring, as it would make this extremely confusing to review: #31892
It branches out from this PR, and only takes care of removing BlockNavigation and replacing it with BlockNavigationTree.
7 tasks
Copons
force-pushed
the
try/simplify-block-navigation
branch
3 times, most recently
from
ed7e1e9 to
c3f86f5
Compare
7 tasks
ntsekouras
reviewed
May 17, 2021
| * @param {Array} props.blocks Custom subset of block client IDs to be used instead of the default hierarchy. | ||
| * @param {Function} props.onSelect Block selection callback. | ||
| * @param {Array} props.selectedBlockClientIds The client IDs of the (single or multi-) selected blocks. | ||
| * @param {boolean} props.showNestedBlocks Flag to enable displaying nested blocks. |
There was a problem hiding this comment.
Should we expose/handle this property explicitly and set it as default to true? Currently we have four usages of this component where all pass this prop with true.
There was a problem hiding this comment.
Even though we almost have this enabled everywhere, personally I'd still prefer to opt-in rather than opt-out.
There was a problem hiding this comment.
I don't have a strong opinion either way, to be honest.
I haven't paid much attention to that prop; it has been there for quite a while now, and I have no idea why it was introduced instead of just defaulting it to true. 🤔
packages/block-editor/src/components/block-navigation/use-block-navigation-client-ids.js
Outdated
Show resolved
Hide resolved
| return selectedBlockClientIds; | ||
| } | ||
|
|
||
| if ( __experimentalPersistentListViewFeatures ) { |
There was a problem hiding this comment.
Does this make any difference somewhere? getSelectedBlockClientIds will first check if it's single block selected and return it in an Array.
There was a problem hiding this comment.
This is a confusing function 😬 I'd expect it to return an Array in all cases. Yet we return a single string on line 32.
We should just use getSelectedBlockClientIds() directly. We can also remove all the Array.isArray checks and just check for length property.
There was a problem hiding this comment.
Well what do you know, I think you're right! 😄
There was a problem hiding this comment.
Or actually...
The problem here is that some consumers (Dropdown to be specific) really expect the selected client ID to be single and string, or null otherwise, whereas the Persistent List View wants the whole multiselection array.
For example, getBlockHierarchyRootClientId (used here and defined here) will use the parameter straight away as object property.
Using those two distinct functions basically saves a bunch of additional checks and conversions to make sure everything keeps receiving the parameters with the expected types.
This said, I think the major cause of confusion here is this:
const isSingleBlockSelected =
selectedClientIds && ! Array.isArray( selectedClientIds );Single selections can be arrays as well, and the naming here doesn't make it clear.
I guess isNotMultiselectedBlock would not make it much clearer, so I'm more oriented to just adding a comment to add some context.
| return selectedBlockClientIds; | ||
| } | ||
|
|
||
| if ( __experimentalPersistentListViewFeatures ) { |
There was a problem hiding this comment.
This is a confusing function 😬 I'd expect it to return an Array in all cases. Yet we return a single string on line 32.
We should just use getSelectedBlockClientIds() directly. We can also remove all the Array.isArray checks and just check for length property.
| * @param {Array} props.blocks Custom subset of block client IDs to be used instead of the default hierarchy. | ||
| * @param {Function} props.onSelect Block selection callback. | ||
| * @param {Array} props.selectedBlockClientIds The client IDs of the (single or multi-) selected blocks. | ||
| * @param {boolean} props.showNestedBlocks Flag to enable displaying nested blocks. |
There was a problem hiding this comment.
Even though we almost have this enabled everywhere, personally I'd still prefer to opt-in rather than opt-out.
david-szabo97
approved these changes
May 20, 2021
Addison-Stavlo
approved these changes
May 20, 2021
|
Thanks folks! When tests clear, I'll go ahead and merge this, and later the restructuring follow up. @ntsekouras The points you raised in your review can be addressed separately, if you think they are a big deal. |
ntsekouras
force-pushed
the
try/simplify-block-navigation
branch
from
f1efb98 to
757f20b
Compare
ntsekouras
approved these changes
May 20, 2021
fullofcaffeine
added a commit
that referenced
this pull request
May 22, 2021
…-take-2 * trunk: (57 commits) Image block: fix cover transform and excessive re-rendering (#32102) compose: Add types to useMergeRefs (#31939) Navigation: Fix collapsing regression. (#32081) components: Promote Elevation (#31614) compose: Add types to useReducedMotion and useMediaQuery (#31941) Update the graphic that appears in the Template Editor welcome guide (#32055) Block Navigation: use CSS for indentation with known max indent instead of spacer divs (#32063) Fix broken template part converter modal styles. (#32097) compose: Add types to `usePrevious` (#31944) components: Add ZStack (#31613) components: Fix Shortcut polymorphism (#31555) compose: Add types to `useFocusReturn` (#31949) compose: Add types to `useDebounce` (#32015) List View: Simplify the BlockNavigation component (#31290) Remove query context leftovers (#32093) Remove filter_var from blocks (#32046) Templates: Remove now-obsolete gutenberg_get_template_paths() (#32066) [RNMobile] Enable reusable block only in WP.com sites (#31744) Rename ViewOwnProps to PolymorphicComponentProps (#32053) Rich text: remove inline display warning (#32013) ...
youknowriad
added
the
Backport to WP 6.7 Beta/RC
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
May 31, 2021
Simplify the `BlockNavigation` component, moving most of the logic inside its `BlockNavigationTree` sub-component.
5 tasks
nylen
pushed a commit
to nylen/wordpress-develop-svn
that referenced
this pull request
Jun 1, 2021
This includes the following fixes: - Generate babel polyfill dynamically WordPress/gutenberg#31279 - Improve the List View component WordPress/gutenberg#31290 WordPress/gutenberg#32063 - Template mode: - Fix embed dimensions WordPress/gutenberg#32057 - Update the welcome guide WordPress/gutenberg#32055 WordPress/gutenberg#32026 - Don’t display the notice at the same time as the welcome guide WordPress/gutenberg#32076 - Remove MetaBoxes WordPress/gutenberg#32315 - Update the title area WordPress/gutenberg#32037 - Widgets Screen: - Fix unsaved changes WordPress/gutenberg#31757 - Fix toolbar alignment WordPress/gutenberg#31991 - Fix block toolbar position after scroll WordPress/gutenberg#32212 - Fix the visible widget area header WordPress/gutenberg#32262 - Fix legacy widgets preview WordPress/gutenberg#32260 - - Block Widgets in the customizer: - Fix customizer title overlapping block toolbar WordPress/gutenberg#32140 - Fix styling issues WordPress/gutenberg#32072 - Fix escape key events WordPress/gutenberg#32175 - Add preferences menu group label WordPress/gutenberg#32259 - Fix creating and replacing legacy widgets WordPress/gutenberg#32005 - Fix the welcome guide’s image WordPress/gutenberg#32264 WordPress/gutenberg#32302 - Fix Cover to Image transform duotone error WordPress/gutenberg#32006 - Remove filter_var usage from blocks WordPress/gutenberg#32046 - Fix image width for aligned Post Featured Image block WordPress/gutenberg#32070 - Prevent excessive Image block re-rendering WordPress/gutenberg#32102 - Remove gutenberg domain from core blocks WordPress/gutenberg#32152 - Use the block editor context class for the the different settings filters WordPress/gutenberg#32159 - Fix Latest Posts block grid view WordPress/gutenberg#32160 - Fix preset classes generation per block WordPress/gutenberg#32190 - Fix logic to enable custom colors and gradients WordPress/gutenberg#32200 - Update the Site Logo logic to use a dedicated site option WordPress/gutenberg#32229 - Limit the Latest Posts block’s featured image width WordPress/gutenberg#32245 - Remove opacity animation in the canvas. WordPress/gutenberg#32266 - Make the focus style valid CSS WordPress/gutenberg#32305 - Fix theme.json styles for the core/list block WordPress/gutenberg#32343 - Fix PHP notice when calling render_block WordPress/gutenberg#32135 Props nosolosw, noisysocks. See #52991. git-svn-id: https://develop.svn.wordpress.org/trunk@51051 602fd350-edb4-49c9-b593-d223f7449a82
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Jun 1, 2021
This includes the following fixes: - Generate babel polyfill dynamically WordPress/gutenberg#31279 - Improve the List View component WordPress/gutenberg#31290 WordPress/gutenberg#32063 - Template mode: - Fix embed dimensions WordPress/gutenberg#32057 - Update the welcome guide WordPress/gutenberg#32055 WordPress/gutenberg#32026 - Don’t display the notice at the same time as the welcome guide WordPress/gutenberg#32076 - Remove MetaBoxes WordPress/gutenberg#32315 - Update the title area WordPress/gutenberg#32037 - Widgets Screen: - Fix unsaved changes WordPress/gutenberg#31757 - Fix toolbar alignment WordPress/gutenberg#31991 - Fix block toolbar position after scroll WordPress/gutenberg#32212 - Fix the visible widget area header WordPress/gutenberg#32262 - Fix legacy widgets preview WordPress/gutenberg#32260 - - Block Widgets in the customizer: - Fix customizer title overlapping block toolbar WordPress/gutenberg#32140 - Fix styling issues WordPress/gutenberg#32072 - Fix escape key events WordPress/gutenberg#32175 - Add preferences menu group label WordPress/gutenberg#32259 - Fix creating and replacing legacy widgets WordPress/gutenberg#32005 - Fix the welcome guide’s image WordPress/gutenberg#32264 WordPress/gutenberg#32302 - Fix Cover to Image transform duotone error WordPress/gutenberg#32006 - Remove filter_var usage from blocks WordPress/gutenberg#32046 - Fix image width for aligned Post Featured Image block WordPress/gutenberg#32070 - Prevent excessive Image block re-rendering WordPress/gutenberg#32102 - Remove gutenberg domain from core blocks WordPress/gutenberg#32152 - Use the block editor context class for the the different settings filters WordPress/gutenberg#32159 - Fix Latest Posts block grid view WordPress/gutenberg#32160 - Fix preset classes generation per block WordPress/gutenberg#32190 - Fix logic to enable custom colors and gradients WordPress/gutenberg#32200 - Update the Site Logo logic to use a dedicated site option WordPress/gutenberg#32229 - Limit the Latest Posts block’s featured image width WordPress/gutenberg#32245 - Remove opacity animation in the canvas. WordPress/gutenberg#32266 - Make the focus style valid CSS WordPress/gutenberg#32305 - Fix theme.json styles for the core/list block WordPress/gutenberg#32343 - Fix PHP notice when calling render_block WordPress/gutenberg#32135 Props nosolosw, noisysocks. See #52991. git-svn-id: https://develop.svn.wordpress.org/trunk@51051 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Jun 1, 2021
This includes the following fixes: - Generate babel polyfill dynamically WordPress/gutenberg#31279 - Improve the List View component WordPress/gutenberg#31290 WordPress/gutenberg#32063 - Template mode: - Fix embed dimensions WordPress/gutenberg#32057 - Update the welcome guide WordPress/gutenberg#32055 WordPress/gutenberg#32026 - Don’t display the notice at the same time as the welcome guide WordPress/gutenberg#32076 - Remove MetaBoxes WordPress/gutenberg#32315 - Update the title area WordPress/gutenberg#32037 - Widgets Screen: - Fix unsaved changes WordPress/gutenberg#31757 - Fix toolbar alignment WordPress/gutenberg#31991 - Fix block toolbar position after scroll WordPress/gutenberg#32212 - Fix the visible widget area header WordPress/gutenberg#32262 - Fix legacy widgets preview WordPress/gutenberg#32260 - - Block Widgets in the customizer: - Fix customizer title overlapping block toolbar WordPress/gutenberg#32140 - Fix styling issues WordPress/gutenberg#32072 - Fix escape key events WordPress/gutenberg#32175 - Add preferences menu group label WordPress/gutenberg#32259 - Fix creating and replacing legacy widgets WordPress/gutenberg#32005 - Fix the welcome guide’s image WordPress/gutenberg#32264 WordPress/gutenberg#32302 - Fix Cover to Image transform duotone error WordPress/gutenberg#32006 - Remove filter_var usage from blocks WordPress/gutenberg#32046 - Fix image width for aligned Post Featured Image block WordPress/gutenberg#32070 - Prevent excessive Image block re-rendering WordPress/gutenberg#32102 - Remove gutenberg domain from core blocks WordPress/gutenberg#32152 - Use the block editor context class for the the different settings filters WordPress/gutenberg#32159 - Fix Latest Posts block grid view WordPress/gutenberg#32160 - Fix preset classes generation per block WordPress/gutenberg#32190 - Fix logic to enable custom colors and gradients WordPress/gutenberg#32200 - Update the Site Logo logic to use a dedicated site option WordPress/gutenberg#32229 - Limit the Latest Posts block’s featured image width WordPress/gutenberg#32245 - Remove opacity animation in the canvas. WordPress/gutenberg#32266 - Make the focus style valid CSS WordPress/gutenberg#32305 - Fix theme.json styles for the core/list block WordPress/gutenberg#32343 - Fix PHP notice when calling render_block WordPress/gutenberg#32135 Props nosolosw, noisysocks. See #52991. Built from https://develop.svn.wordpress.org/trunk@51051 git-svn-id: http://core.svn.wordpress.org/trunk@50660 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott
pushed a commit
to gMagicScott/core.wordpress-mirror
that referenced
this pull request
Jun 1, 2021
This includes the following fixes: - Generate babel polyfill dynamically WordPress/gutenberg#31279 - Improve the List View component WordPress/gutenberg#31290 WordPress/gutenberg#32063 - Template mode: - Fix embed dimensions WordPress/gutenberg#32057 - Update the welcome guide WordPress/gutenberg#32055 WordPress/gutenberg#32026 - Don’t display the notice at the same time as the welcome guide WordPress/gutenberg#32076 - Remove MetaBoxes WordPress/gutenberg#32315 - Update the title area WordPress/gutenberg#32037 - Widgets Screen: - Fix unsaved changes WordPress/gutenberg#31757 - Fix toolbar alignment WordPress/gutenberg#31991 - Fix block toolbar position after scroll WordPress/gutenberg#32212 - Fix the visible widget area header WordPress/gutenberg#32262 - Fix legacy widgets preview WordPress/gutenberg#32260 - - Block Widgets in the customizer: - Fix customizer title overlapping block toolbar WordPress/gutenberg#32140 - Fix styling issues WordPress/gutenberg#32072 - Fix escape key events WordPress/gutenberg#32175 - Add preferences menu group label WordPress/gutenberg#32259 - Fix creating and replacing legacy widgets WordPress/gutenberg#32005 - Fix the welcome guide’s image WordPress/gutenberg#32264 WordPress/gutenberg#32302 - Fix Cover to Image transform duotone error WordPress/gutenberg#32006 - Remove filter_var usage from blocks WordPress/gutenberg#32046 - Fix image width for aligned Post Featured Image block WordPress/gutenberg#32070 - Prevent excessive Image block re-rendering WordPress/gutenberg#32102 - Remove gutenberg domain from core blocks WordPress/gutenberg#32152 - Use the block editor context class for the the different settings filters WordPress/gutenberg#32159 - Fix Latest Posts block grid view WordPress/gutenberg#32160 - Fix preset classes generation per block WordPress/gutenberg#32190 - Fix logic to enable custom colors and gradients WordPress/gutenberg#32200 - Update the Site Logo logic to use a dedicated site option WordPress/gutenberg#32229 - Limit the Latest Posts block’s featured image width WordPress/gutenberg#32245 - Remove opacity animation in the canvas. WordPress/gutenberg#32266 - Make the focus style valid CSS WordPress/gutenberg#32305 - Fix theme.json styles for the core/list block WordPress/gutenberg#32343 - Fix PHP notice when calling render_block WordPress/gutenberg#32135 Props nosolosw, noisysocks. See #52991. Built from https://develop.svn.wordpress.org/trunk@51051 git-svn-id: https://core.svn.wordpress.org/trunk@50660 1a063a9b-81f0-0310-95a4-ce76da25c4cd
F-Wilke
pushed a commit
to FiliagoDev/WordPress
that referenced
this pull request
Jul 31, 2021
This includes the following fixes: - Generate babel polyfill dynamically WordPress/gutenberg#31279 - Improve the List View component WordPress/gutenberg#31290 WordPress/gutenberg#32063 - Template mode: - Fix embed dimensions WordPress/gutenberg#32057 - Update the welcome guide WordPress/gutenberg#32055 WordPress/gutenberg#32026 - Don’t display the notice at the same time as the welcome guide WordPress/gutenberg#32076 - Remove MetaBoxes WordPress/gutenberg#32315 - Update the title area WordPress/gutenberg#32037 - Widgets Screen: - Fix unsaved changes WordPress/gutenberg#31757 - Fix toolbar alignment WordPress/gutenberg#31991 - Fix block toolbar position after scroll WordPress/gutenberg#32212 - Fix the visible widget area header WordPress/gutenberg#32262 - Fix legacy widgets preview WordPress/gutenberg#32260 - - Block Widgets in the customizer: - Fix customizer title overlapping block toolbar WordPress/gutenberg#32140 - Fix styling issues WordPress/gutenberg#32072 - Fix escape key events WordPress/gutenberg#32175 - Add preferences menu group label WordPress/gutenberg#32259 - Fix creating and replacing legacy widgets WordPress/gutenberg#32005 - Fix the welcome guide’s image WordPress/gutenberg#32264 WordPress/gutenberg#32302 - Fix Cover to Image transform duotone error WordPress/gutenberg#32006 - Remove filter_var usage from blocks WordPress/gutenberg#32046 - Fix image width for aligned Post Featured Image block WordPress/gutenberg#32070 - Prevent excessive Image block re-rendering WordPress/gutenberg#32102 - Remove gutenberg domain from core blocks WordPress/gutenberg#32152 - Use the block editor context class for the the different settings filters WordPress/gutenberg#32159 - Fix Latest Posts block grid view WordPress/gutenberg#32160 - Fix preset classes generation per block WordPress/gutenberg#32190 - Fix logic to enable custom colors and gradients WordPress/gutenberg#32200 - Update the Site Logo logic to use a dedicated site option WordPress/gutenberg#32229 - Limit the Latest Posts block’s featured image width WordPress/gutenberg#32245 - Remove opacity animation in the canvas. WordPress/gutenberg#32266 - Make the focus style valid CSS WordPress/gutenberg#32305 - Fix theme.json styles for the core/list block WordPress/gutenberg#32343 - Fix PHP notice when calling render_block WordPress/gutenberg#32135 Props nosolosw, noisysocks. See #52991. Built from https://develop.svn.wordpress.org/trunk@51051 git-svn-id: http://core.svn.wordpress.org/trunk@50660 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
BlockNavigationcomponent, moving most of the logic inside itsBlockNavigationTreesub-component.This change was initiated after a conversation that surfaced how the
BlockNavigationcomponent is currently used.Apparently,
BlockNavigationis not intended to be used directly (it's not even exported by the package), but indirectly as rendered content of theBlockNavigationDropdown's dropdown, which was normally designed as List View dropdown for the Post Editor toolbar.This makes the "index" component very opinionated as a dropdown content. It renders a dropdown label, it only shows the blocks belonging to the currently selected hierarchy, and so on.
In time, other consumers wanted to make use of the List View, but with different opinions.
Avoiding the "index" component, they started using
BlockNavigationTreedirectly.The
Navigationblock, for example, wanted a list starting from a specific client ID.The Persistent List View wanted all the blocks, regardless of the hierarchy, and a range of other options.
Now that we are in the process of extending the Persistent List View from the Site Editor to the Post Editor as well, we realized that the only consumer using the List View as it was originally intended (
BlockNavigationcalled throughBlockNavigationDropdown) is the Widgets Editor.Everyone else is hacking around it by experimentally using
BlockNavigationTree.In this PR I try to acknowledge this diversity, and move all the blocks logic inside
BlockNavigationTree, while maintaining the current format and extensibility.BlockNavigationTreeis now directly responsible to retrieve the block list, the selected block (or blocks), etc.BlockNavigationTreehas a newonlyShowCurrentHierarchyprop to limit the list to only the currently selected hierarchy (e.g. if the selected block is nested inside a group, the list will show up to that group, instead of up to the document root). This is a breaking change: this behaviour used to be the default, while now it requires a flag.BlockNavigationTreeselections by providing their own specific data through props.Possible follow ups:
BlockNavigationTreethe "index" component, to better reflect how it's actually used.BlockNavigationas we know it today. Move the leftovers (basically only the dropdown label, I think) intoBlockNavigationDropdown.How has this been tested?
Smoke test the various List View implementations:
Types of changes
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Checklist:
*.native.jsfiles for terms that need renaming or removal).