Tidy list view props and deprecate BlockNavigationDropdown
#40777
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
talldan
added
[Type] Code Quality
talldan
requested review from
draganescu,
noisysocks,
tellthemachines,
adamziel,
kevin940726 and
ellatrix
as code owners
|
Size Change: -206 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
talldan
changed the title
BlockNavigationDropdown
getdave
approved these changes
May 3, 2022
|
These changes all make sense to me. My only concern (or question) is if we remove some of the experimental props and enable them everywhere, wouldn't that mean this PR is introducing breaking changes by enabling features by default? I can see that you've already updated the CHANGELOG though, just wondering if we should make this clearer? (Maybe they're already covered in the previous entries in the CHANGELOG?) |
@kevin940726 There's only one place where that would be the case - All the other usages are in components aren't exported where the props were |
|
Also the entire component is experimental. I reckon we should promote it to a full API. Why is it even experimental at all at this stage? It's heavily used. |
talldan
force-pushed
the
tidy/list-view-props
branch
from
6a4aa35 to
f61fa0d
Compare
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.
What?
Improves the
ListViewcomponent's props, which were a bit untidy, particularly the__experimentalprops:__experimentalFeatures- which was first used to test some experimental features for List View that were being developed in the navigation editor. All of these features have either now been removed or are considered stable. This prop wastrueon every ListView instance in this project, so I think it's fine to remove the prop and consider and consider all the features tied to it as active by default (block settings menu).__experimentalPersistentListViewFeatures- Similarly, this was used to make List View behave differently when the persistent sidebar was first introduced. The persistent sidebar is now the norm, so it seems fine to remove the prop. It wastrue(almost) everywhere, and with this PR all the features tied to it are active by default (windowing, and better block highlights).__experimentalHideContainerBlockActions- this was used by the Widget editor to hide the Block Settings menu for widget areas. I don't think the way it works was ideal. Widget Areas already use the__experimentalToolbar: falseblock supports setting to hide the block toolbar and block settings menu in the editor canvas, so I've switched List View to use this as well.showNestedBlocks- this was alwaystrueeverywhere ListView was used, and I don't think there's any time a list view would be shown without nested blocks now. I think it'd be good to make ittrueby default....props- I don't fully understand why this was there, it allowed any props to be set onListViewBranch. I would expect it to be applied to the root element if anything. It was unused.This also deprecates the old
BlockNavigationDropdowncomponent.Why?
Code quality / DevEx.
List View itself is an experimental component, but I think it's worth improving the public API and looking at making it stable.
Testing Instructions
Everything should be the same as in
trunk.