List View: Restructure the BlockNavigation component #31892
Merged
Conversation
This file contains hidden or 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
Copons
requested review from
Addison-Stavlo,
creativecoder,
david-szabo97,
jeyip,
mattwiebe,
ntsekouras,
talldan and
vindl
7 tasks
|
Size Change: +241 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
david-szabo97
approved these changes
May 20, 2021
Member
david-szabo97
left a comment
david-szabo97
left a comment
There was a problem hiding this comment.
Changes make sense to me. Works as expected! ✅
Addison-Stavlo
approved these changes
May 20, 2021
Contributor
Addison-Stavlo
left a comment
Addison-Stavlo
left a comment
There was a problem hiding this comment.
This also tests well, and the restructure looks great to me!
ntsekouras
force-pushed
the
try/simplify-block-navigation
branch
from
f1efb98 to
757f20b
Compare
Contributor
Author
|
Just a note that I'll rebase this and sort the conflicts out once #31290 goes in. |
talldan
reviewed
May 21, 2021
talldan
reviewed
May 21, 2021
| export { default as BlockEdit, useBlockEditContext } from './block-edit'; | ||
| export { default as BlockIcon } from './block-icon'; | ||
| export { default as BlockNavigationDropdown } from './block-navigation/dropdown'; | ||
| export { default as __experimentalBlockNavigation } from './block-navigation'; |
Contributor
There was a problem hiding this comment.
It'd be great to call this ListView or similar. Maybe it could all be moved to a list-view folder leaving only the dropdown in the block navigation folder (which might also be a candidate for deprecation).
That is quite a big change, so definitely optional!
Contributor
Author
There was a problem hiding this comment.
This is actually a great idea. 🤔
I'll try pushing a commit and see if git manages to keep the diff readable, otherwise I'll move to a follow up.
Contributor
Author
There was a problem hiding this comment.
Ooof, it went from +10-10 to +400-400 🤣
Copons
force-pushed
the
try/restructure-block-navigation
branch
from
67ecaca to
2784f50
Compare
Contributor
Author
|
As suggested by @talldan I've taken the chance here of renaming the whole I've left Let me know what you think. |
talldan
approved these changes
May 26, 2021
| const getSlotName = ( clientId ) => `ListViewBlock-${ clientId }`; | ||
|
|
||
| function BlockNavigationBlockSlot( props, ref ) { | ||
| function ListViewBlockSlot( props, ref ) { |
Contributor
There was a problem hiding this comment.
The Slot/Fill could be a candidate for deletion in the future. It was an experiment that didn't really go anywhere, but it's removal would help simplify the code quite a bit.
Replace the deprecated BlockNavigationTree with BlockNavigation Remove the deprecated BlockNavigationTree component Try renaming into ListView More renaming!
talldan
force-pushed
the
try/restructure-block-navigation
branch
from
13528cd to
29c63c2
Compare
Contributor
|
Hope it's not too presumptuous, but I wanted to get this one merged, so I've rebased, fixed conflicts and will merge it when tests pass. |
Contributor
Author
|
Thank you so much @talldan! |
sarayourfriend
pushed a commit
that referenced
this pull request
Jul 15, 2021
* Use BlockNavigationTree as main component Replace the deprecated BlockNavigationTree with BlockNavigation Remove the deprecated BlockNavigationTree component Try renaming into ListView More renaming! * Update README title Co-authored-by: Daniel Richards <[email protected]>
12 tasks
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
Follow up to #31290.
While working on the list view it became clear that the
BlockNavigationcomponent is improperly structured for its current use cases.The main component is not exported in the package, but only used internally by
BlockNavigationDropdown, which is exported as a stable component.At the same time,
BlockNavigationTreeis exported (experimentally), used in multiple different packages, and even internally byBlockNavigationitself.As far as I can see, there is no reason to keep
BlockNavigation, and instead there are many reasons to turnBlockNavigationTreeas the main component.Proposed changes:
BlockNavigationcomponent, and "merge" it intoBlockNavigationDropdown.BlockNavigationTreeintoBlockNavigation.BlockNavigationTree: it can still be used with the old name and from the old path, but it throws a deprecation warning in the console.BlockNavigationTreeintoBlockNavigation.Notes
There was no need of updating the component's readme, as it works well enough for the new usage.
Although, the screenshots should be updated, as they refer to the dropdown version, which is now not the primary way of using this component.
The screenshots are uploaded to make.wordpress.org, but I don't know the actual procedure.
How has this been tested?
Smoke test the List View in: Post Editor, Site Editor, Navigation block.
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).