Skip to content

Legacy Widget: Improve backwards compatibility #30709

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.

Sign up for GitHub

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

Merged
noisysocks merged 10 commits into from
Apr 15, 2021
Merged

Legacy Widget: Improve backwards compatibility #30709

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
730581f
Legacy Widget: Improve backwards compatibility
noisysocks Apr 9, 2021
f980db7
Fix Legacy Widget E2E test
noisysocks Apr 13, 2021
6d697e3
Add onSave to deps
noisysocks Apr 14, 2021
0c42b1a
Add setFormData to deps
noisysocks Apr 14, 2021
4d591c5
Import jquery instead of accessing from window
noisysocks Apr 14, 2021
a199fee
Remove unnecessary useCallback
noisysocks Apr 14, 2021
968cb98
Widgets E2E tests: Query by label instead of id
noisysocks Apr 14, 2021
6196284
Replace querySelector with extra ref
noisysocks Apr 14, 2021
eda650c
Revert "Import jquery instead of accessing from window"
noisysocks Apr 14, 2021
216e286
Add fallback if jQuery does not exist
noisysocks Apr 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Import jquery instead of accessing from window
  • Loading branch information
@noisysocks
noisysocks committed Apr 14, 2021
commit 4d591c5ed18408656a1bbd2e6a31ba39df2149bf
6 changes: 6 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/block-library/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"@wordpress/viewport": "file:../viewport",
"classnames": "^2.2.5",
"fast-average-color": "4.3.0",
"jquery": "^3.5.1",
Copy link
Member

@kevin940726 kevin940726 Apr 14, 2021

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?

Copy link
Member Author

@noisysocks noisysocks Apr 14, 2021

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.

Copy link
Contributor

@youknowriad youknowriad Apr 14, 2021

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

Copy link
Member

@gziolo gziolo Apr 14, 2021

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

Copy link
Contributor

@draganescu draganescu Apr 14, 2021

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.

Copy link
Member Author

@noisysocks noisysocks Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes me wonder whether the legacy widget block should be in the block library package or in edit-widgets

It was in edit-widgets but I moved it back to block-library in #29960 for two reasons:

  • It means that customize-widgets can use the block.
  • It ensures that we are not relying on anything in 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.

The legacy widget block is tied to the way widgets are stored

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.

"lodash": "^4.17.19",
"memize": "^1.1.0",
"moment": "^2.22.1",
Expand Down
3 changes: 1 addition & 2 deletions packages/block-library/src/legacy-widget/edit/form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { debounce } from 'lodash';
import $ from 'jquery';

/**
* WordPress dependencies
Expand All @@ -20,8 +21,6 @@ import apiFetch from '@wordpress/api-fetch';
import { Button } from '@wordpress/components';
import { useInstanceId } from '@wordpress/compose';

const $ = window.jQuery;

export default function Form( { id, idBase, instance, setInstance } ) {
const { html, setFormData } = useForm( {
id,
Expand Down