Add @wordpress/menus package
#31586
Closed
Add @wordpress/menus package
#31586
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
getdave
added
[Block] Navigation
|
I feel this PR now has enough changes to demonstrate the utility of this package. I won't take things further (although I could) based on the view that we should merge early and then continue to iterate. |
getdave
requested review from
adamziel,
ajitbohra,
chrisvanpatten,
draganescu,
kevin940726,
noisysocks and
tellthemachines
as code owners
|
Size Change: -834 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
It's very important to have a good plan for what this new package would include. All constants, methods, and components exposed from this package will become public API for the WordPress core that we need to support indefinitely. They will be exposed under |
|
Also, is this a package that can be considered WP Agnostic (most packages should be ideally). I personally don't like packages that are just there to separate things and hold a bunch of reusable stuff, a package should have a clear purpose and can be used on its own do to something meaningful. |
|
@youknowriad It is indeed separating things and holding reusable code.
Given the code duplication and potential for regression having lots of duplicate code introduces (not always but in this case I believe it does/has already) what other avenues does the Gutenberg project provide allow us to share code between packages? We already have this pattern in play in:
As such it feels like there is a need for a means of code reuse across modules. If packages are the way forward then it appears there is a argument for introducing a different mechanism - perhaps one that isn't published to the registry? |
|
FYI this Issue #31599 highlights one the main use cases for needing to refactor. |
This comment has been minimized.
This comment has been minimized.
talldan
reviewed
May 10, 2021
There was a problem hiding this comment.
May not be useful, but I just remembered the other idea I had for refactoring this code, which was to treat the block <--> menu item conversion as a block transform with a 'menuItem' type (or perhaps a way to describe an entity to block transform? Page to Nav Link may also be useful).
There are a few aspects that I was unclear on, so I hadn't pushed on with the idea:
- Is this too specific for the Navigation Link block? It might end up being the only block that has this transform. Should third party blocks be able to add such a transform type?
- How could this be used for back compat/extensibility? (e.g. extending the conversion process to allow converting custom menu item fields to block attributes, which might relate to Navigation Screen: Menu item custom ACF options #31551). At the moment I don't know enough about how menu item custom fields work—needs more investigation.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gwwar
reviewed
May 10, 2021
|
Leaving a couple of high level thoughts here: Code Duplication leads to driftCode duplication is generally a good first step to avoid tight coupling when we're still exploring a problem space. However this also means that implementations will drift over time, regardless of how diligent folks might be. This can happen very easily, for example new contributors come in to fix bug but don't realize that this code exists in many other places. In some cases the drift is fine since we realize that use cases will differ, but if that isn't the case and we'd like to keep logic in sync, some shared place to store the code is necessary. Many Small Packages (per feature) vs One Shared Package?While It's likely that there will be other new features that need to share common logic or default implementations across our block editors (site / post / widget / navigation / any new ones we can build). There was a bit of exploration in #29993 for this, but a single experimental function wasn't a great candidate. NamingIf we do choose to pick many small packages, does Menus work for us? If I recall correctly the Navigation block was named Navigation over Menu to avoid confusion over restaurant menus. __experimentalFetchLinkSuggestionsThis currently resides in |
|
Thanks, everyone for your valuable input. Somehow this PR became the place to discuss the merit of the feature. My fault. Let's move further discussion to the Issue where I will summarize thoughts and respond to Matias's question. |
getdave
force-pushed
the
add/menus-package
branch
from
9546100 to
70c01ed
Compare
|
We're probably not going to pursue this at this point. If we need to we can revisit. Closing for now. |
7 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
Introduces a
@wordpress/menuspackage as a place to store shared components in preparation for adding a unique placeholder state to the Navigation editor screen.As this placeholder will require much of the same logic, data fetching and mechanics as the block placeholder it makes sense to centralise what we can.
Closes #31580
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).