[Earn] Remove jQuery dependency from Thickbox #28318

millerf · 2023-01-12T13:00:15Z

Fixes Automattic/wp-calypso#71048
See #32237

Proposed changes:

Build our own alternative, jQuery-less version of Thickbox for rendering some of our blocks on the frontend.
Alternative approach to Payments Blocks; remove thickbox #27921
Relies on D95805-code

Jetpack product discussion

p9dueE-6Cp-p2

Does this pull request change what data or activity we track or use?

No

Testing instructions:

See p81Rsd-1am-p2

github-actions · 2023-01-12T13:01:26Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
⚠️ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

Jetpack plugin:

Next scheduled release: August 23, 2023.
Scheduled code freeze: August 21, 2023.

github-actions · 2023-01-12T13:01:28Z

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run

bin/jetpack-downloader test jetpack test/remove-thickbox

to get started. More details: p9dueE-5Nn-p2

artpi

I like this approach, but let's trim this code as much as possible!

Works well, mostly
Code needs trimming
Loading animation has a wrong URL:

projects/plugins/jetpack/extensions/blocks/donations/donations.php

projects/plugins/jetpack/extensions/shared/memberships-modal.js

projects/plugins/jetpack/extensions/shared/memberships.js

artpi · 2023-01-17T14:08:55Z

projects/plugins/jetpack/extensions/shared/memberships.js


 		// This line has to come after the Thickbox has opened otherwise Firefox doesn't scroll to the top.
 		window.scrollTo( 0, 0 );
 	} );
 }

 export const initializeMembershipButtons = selector => {
-	const membershipButtons = Array.prototype.slice.call( document.querySelectorAll( selector ) );
+	const membershipButtons = [ ...document.querySelectorAll( selector ) ];


Check if this code is transpiled

Same as above

Same as above. The fact that your browser supports it does not mean every will

I don't know. I'll change it.

projects/plugins/jetpack/modules/memberships/class-jetpack-memberships.php

artpi · 2023-01-17T15:03:17Z

Also needs rebasing and proper labels and stuff

millerf · 2023-01-18T13:41:31Z

Loading animation has a wrong URL:

I have NO IDEA where to add static data.

artpi · 2023-01-18T13:46:29Z

I have NO IDEA where to add static data.

You need to remove it as we have one displayed in https://github.com/Automattic/jetpack/blob/trunk/projects/plugins/jetpack/extensions/shared/memberships.scss#L9

jeherve

Any chance you could add some testing instructions, so I'm sure not to miss anything when I test this?

projects/plugins/jetpack/extensions/blocks/subscriptions/subscriptions.php

projects/plugins/jetpack/extensions/shared/memberships-modal.js

jeherve · 2023-01-23T16:17:12Z

projects/plugins/jetpack/extensions/shared/memberships.scss

@@ -37,3 +37,151 @@
 BODY.modal-open {
 	overflow: hidden;
 }
+
+/* ----------------------------------------------------------------------------------------------------------------*/


Should this CSS be moved to a central location, where other extensions using the thickbox can use it too?

Not sure we want to share this library just yet: this is a really down-sized (in term of feature) version of thickbox, and there is no documentation. Maybe it should stay with this extension...
But we can definitely start a talk about re-using and re-implementing the rest of the functionnalities if JP is eager to use this...

jeherve · 2023-01-23T16:33:32Z

projects/plugins/jetpack/extensions/shared/memberships-modal.js

+					'beforeend',
+					"<div id='TB_title'><div id='TB_ajaxWindowTitle'>" +
+						caption +
+						"</div><div id='TB_closeAjaxWindow'><a href='#' id='TB_closeWindowButton' title='Close'>close</a> or Esc Key</div></div><iframe frameborder='0' hspace='0' src='" +


It would be nice if those strings were translated. Since you placed the lib in the extensions dir, it gets transpiled so you should be able to either:

Import __ from @wordpress/i18n` and translate in place. I would not recommend this since it would bring a lot of extra dependencies on a file that we also load on the frontend.

Pass those translations from php to js with wp_localize_script. We do not have a specific script handle to attach it to at the moment though.

This all makes me wonder if that new lib should be handled like a real lib, like some of the ones we have in projects/plugins/jetpack/_inc. We would then register it as a new script (jetpack-thickbox) in Jetpack::register_assets(), and pass the translations there. We could then add the script as a dependency when needed.

To give you a better idea of what I mean, I pushed 79d3e76 in a test branch. Let me know what you think about the idea.

That gos with my comment here.

I do like the idea, but maybe we should wrap this PR first and then extend its functionality and share it for other extensions after talking with the team?

I opened the discussion here:
p9dueE-6Cp-p2

artpi · 2023-01-24T12:37:59Z

To clarify my thoughts on this:

I am against creating a drop-in full-featured replacement for thickbox and especially creating a shared library because:

The long-term solution is to use a modal shipped in Gutenberg anyway once that becomes mature.
The way subscriptions use Thickbox is really hackish. We explicitly turn off certain thickbox features. We would not even know how to test Thickbox in a way that is safe for other modules
Creating our own implementation of Thickbox will let us slowly refactor it to be able to transition to something else (1). Having to support other modules will make it harder.

We can take Fab's work and start the process of creating a shared thickbox replacement as a separate piece of work. However this is not something we can do in the timeline that we have

sgomes · 2023-01-31T14:46:15Z

@artpi : Regarding the long term solution, I want to note that most @wordpress packages aren't intended to be used in the frontend. @wordpress/data, @wordpress/components, and @wordpress/elements, for example, are incorrectly being used in the frontend in some Jetpack blocks, but they come with heavy dependencies that are really only meant to be used in the editor.

#28626 tries to keep tabs on what libraries are being used on frontend JS, to avoid the issue spreading further.

As such, what we really need to replace thickbox is a frontend-safe modal library. Is there anything like that currently planned for Gutenberg?

artpi · 2023-01-31T14:59:02Z

As such, what we really need to replace thickbox is a frontend-safe modal library. Is there anything like that currently planned for Gutenberg?

I will page @mtias as he is the instigator for this project and knows best

n3f · 2023-01-31T19:43:25Z

packages aren't intended to be used in the frontend

I'm going to go and close my PR then 😆.

n3f · 2023-02-03T22:48:41Z

As such, what we really need to replace thickbox is a frontend-safe modal library. Is there anything like that currently planned for Gutenberg?

I will page @mtias as he is the instigator for this project and knows best

Matias answered this in p9dueE-6Cp-p2#comment-9217 with a link about using this: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/navigation/view-modal.js

mtias · 2023-02-04T11:36:49Z

@n3f that's the leanest implementation we have on the front-end. I'd also be curious if we can use <dialog> directly, which is now mature enough and handles all the interaction details. It might be tricky to make it work with an iframe, but it's worth checking since it's the simplest solution.

jeherve · 2023-02-10T09:59:21Z

I updated this PR description and refreshed it with latest trunk so it can be tested, but I'm still not sure on the current status for this. Based on the different discussions on P2 and above, should we go with an implementation of view.modal.js or <dialog>?

github-actions · 2023-07-09T00:51:28Z

This PR has been marked as stale. This happened because:

It has been inactive in the past 3 months.
It hasn’t been labeled `[Pri] BLOCKER`, `[Pri] High`, etc.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with trunk, and it is still valid. Feel free to close this PR if you think it's not valid anymore — if you do, please add a brief explanation.

millerf marked this pull request as draft January 12, 2023 13:00

github-actions bot added [Block] Donations [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Feature] Memberships labels Jan 12, 2023

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 12, 2023

github-actions bot added the [Status] In Progress label Jan 12, 2023

artpi requested changes Jan 17, 2023

View reviewed changes

artpi mentioned this pull request Jan 17, 2023

Payments Blocks; remove thickbox #27921

Closed

3 tasks

millerf added 15 commits January 18, 2023 14:36

[not verified] Setup before porting Thickbox

efc0ba9

[not verified] Removing jQuery from Thickbox

68a33b1

[not verified] More porting

4a49c98

[not verified] More porting

ebe2c11

[not verified] Almost there

0976de0

[not verified] iframe is loading but not displaying

70e6c33

[not verified] Fix iframe width

9f371a1

[not verified] Remove debugger

f817cce

[not verified] Fix some issues

26b6bb0

[not verified] Comment try catch block

c3ab0f2

[not verified] Fix lint

7c89e86

[not verified] Rewrite show()

6564ffe

[not verified] Fix remove

1c93f9f

[not verified] Some fixes

c93da11

[not verified] Prevent scrolling

5975152

millerf force-pushed the test/remove-thickbox branch from 49774f9 to 5975152 Compare January 18, 2023 13:36

millerf removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 18, 2023

jeherve reviewed Jan 23, 2023

View reviewed changes

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 23, 2023

Remove unecessary phpcs rule

e9f92ff

millerf dismissed stale reviews from artpi and romarioraffington via e9f92ff January 24, 2023 09:52

Remove wp_enqueue_style( 'thickbox' );

baef85d

millerf changed the title ~~[Earn] Remove thickbox and jQuery dependency~~ [Earn] Remove jQuery dependency from Thickbox Jan 24, 2023

Merge branch 'trunk' into test/remove-thickbox

39a0a84

millerf added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 25, 2023

jeherve added [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. [Block] Payment Button aka Recurring Payments [Type] Janitorial [Pri] Normal labels Feb 10, 2023

jeherve assigned millerf Feb 10, 2023

Merge remote-tracking branch 'origin/trunk' into test/remove-thickbox

238ce06

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 10, 2023

github-actions bot added the [Status] Stale label Jul 9, 2023

jeherve mentioned this pull request Aug 2, 2023

Subscriptions Block: extra resources loaded every time the block is loaded on a page #32237

Closed

github-actions bot removed the [Status] Stale label Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Earn] Remove jQuery dependency from Thickbox #28318

[Earn] Remove jQuery dependency from Thickbox #28318

millerf commented Jan 12, 2023 •

edited by jeherve

Loading

github-actions bot commented Jan 12, 2023 •

edited

Loading

github-actions bot commented Jan 12, 2023 •

edited

Loading

artpi left a comment

artpi Jan 17, 2023

millerf Jan 18, 2023

artpi Jan 18, 2023

millerf Jan 18, 2023

millerf Jan 18, 2023

artpi commented Jan 17, 2023

millerf commented Jan 18, 2023

artpi commented Jan 18, 2023

jeherve left a comment

jeherve Jan 23, 2023

millerf Jan 24, 2023

jeherve Jan 23, 2023

millerf Jan 24, 2023

millerf Jan 24, 2023

artpi commented Jan 24, 2023

sgomes commented Jan 31, 2023 •

edited

Loading

artpi commented Jan 31, 2023 •

edited

Loading

n3f commented Jan 31, 2023 •

edited

Loading

n3f commented Feb 3, 2023

mtias commented Feb 4, 2023

jeherve commented Feb 10, 2023

github-actions bot commented Jul 9, 2023

[Earn] Remove jQuery dependency from Thickbox #28318

Are you sure you want to change the base?

[Earn] Remove jQuery dependency from Thickbox #28318

Conversation

millerf commented Jan 12, 2023 • edited by jeherve Loading

Proposed changes:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Jan 12, 2023 • edited Loading

github-actions bot commented Jan 12, 2023 • edited Loading

artpi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artpi commented Jan 17, 2023

millerf commented Jan 18, 2023

artpi commented Jan 18, 2023

jeherve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artpi commented Jan 24, 2023

sgomes commented Jan 31, 2023 • edited Loading

artpi commented Jan 31, 2023 • edited Loading

n3f commented Jan 31, 2023 • edited Loading

n3f commented Feb 3, 2023

mtias commented Feb 4, 2023

jeherve commented Feb 10, 2023

github-actions bot commented Jul 9, 2023

millerf commented Jan 12, 2023 •

edited by jeherve

Loading

github-actions bot commented Jan 12, 2023 •

edited

Loading

github-actions bot commented Jan 12, 2023 •

edited

Loading

sgomes commented Jan 31, 2023 •

edited

Loading

artpi commented Jan 31, 2023 •

edited

Loading

n3f commented Jan 31, 2023 •

edited

Loading