-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Legacy Widget: Improve backwards compatibility #30709
Merged
Merged
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
730581f
Legacy Widget: Improve backwards compatibility
noisysocks f980db7
Fix Legacy Widget E2E test
noisysocks 6d697e3
Add onSave to deps
noisysocks 0c42b1a
Add setFormData to deps
noisysocks 4d591c5
Import jquery instead of accessing from window
noisysocks a199fee
Remove unnecessary useCallback
noisysocks 968cb98
Widgets E2E tests: Query by label instead of id
noisysocks 6196284
Replace querySelector with extra ref
noisysocks eda650c
Revert "Import jquery instead of accessing from window"
noisysocks 216e286
Add fallback if jQuery does not exist
noisysocks File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off topic: I always think the version here is misleading, I think it even came up once during my coding test interview 😅. Maybe we should just pin it as
*
to make it more clear that it's not the version we are using at all.However, that would become really weird for direct consumer of the npm package itself. Speaking of that, does it mean we're requiring
jquery
for this package now? I wonder what we could do about it 🤔. Maybe a fallback as you mentioned?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I... do not know 😅
@gziolo: Is this OK? Or is it better to access jQuery via the
window
global? We need to use it for backwards compatibility.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we did a lot of work in the past to avoid jQuery as a dependency in our packages, So I think since this is specifically for BC, we should use the global (if present).
Makes me wonder whether the legacy widget block should be in the block library package or in edit-widgets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question to ask whether the Legacy Widget block would be useful outside of the widgets screen and the Customizer? I wouldn't mind it was moved to the
edit-widgets
package. It's already the case for the Widget Area block:https://github.com/WordPress/gutenberg/tree/trunk/packages/edit-widgets/src/blocks/widget-area
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the legacy widget block should be available anywhere else but the block editor that edits widget areas. The legacy widget block is tied to the way widgets are stored and it would be a bad idea to bring this all over the place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was in
edit-widgets
but I moved it back toblock-library
in #29960 for two reasons:customize-widgets
can use the block.edit-widgets
to implement the block. I think this is important as it gives us the option to, in the future, allow Legacy Widget to be used in the site editor or wherever else we may want to allow it. I could see a case for block-based themes wanting to use a widget when there is no block equivalent.This is not true as of #29960. Care has been taken to make Legacy Widget work like any other block that relies on the REST API: Latest Posts, Latest Comments, etc.