Skip to content
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

Remove jQuery from Recommended Widget #385

Merged
merged 8 commits into from
Sep 20, 2021
Merged

Remove jQuery from Recommended Widget #385

merged 8 commits into from
Sep 20, 2021

Conversation

pauarge
Copy link
Contributor

@pauarge pauarge commented Sep 20, 2021

Description

This is the last PR on the workstream to remove jQuery dependencies on the plugin. With this work, we've replaced all jQuery-implemented elements with vanilla JS.

In this case, we're removing jQuery from the Recommended Widget. It's one single Javascript file plus the enqueue of jQuery itself on the PHP side. We have rebuilt the assets with

npm run build

Motivation and Context

#134

How Has This Been Tested?

This has been tested by generating the new assets on a local environment plus a sandbox and visually checking that the contents of the recommended widget are the same as with the old implementation in different browsers.

Known issues

Neither the new or the old implementation work in Firefox (they do in Safari and Chrome). This will be addressed in a separate issue.

Screenshots (if appropriate):

Screen Shot 2021-09-20 at 11 40 05 AM

Types of changes

Bug fix (non-breaking change which fixes an issue

@pauarge pauarge changed the title Remove jQuery from recommended widget Remove jQuery from Recommended Widget Sep 20, 2021
@pauarge pauarge added this to the 2.6.0 milestone Sep 20, 2021
@pauarge pauarge marked this pull request as ready for review September 20, 2021 09:50
@pauarge pauarge requested a review from a team as a code owner September 20, 2021 09:50
Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

While the proposal looks good, we'll be breaking BC for browser support on the front-end, and that needs some discussion with the Parse.ly folks as that's a business decision.


const textDiv = jQuery( '<div>' ).addClass( 'parsely-text-wrapper' );
fetch( fullUrl )
Copy link
Contributor

Choose a reason for hiding this comment

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

The Fetch API doesn't support IE11, but our defaults for browserslist includes IE11 (run npx browserslist in the plugin directory to get the full list).

Since the plugin still supports WP 4.0, then we either need to make a call and document about the widgets not supporting IE11, or not use / provide a workaround for the Fetch API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to provide a workaround, we could use a Polyfill: https://github.com/github/fetch

However, I'm happy to stop supporting IE11. We can clearly state it in the widget configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Internal stats suggest that IE11 requests account for around 0.1% of requests.

We should also check the behaviour of what the page looks like (is it empty? Does it have a heading but no content? I think there's an issue for this already), but in terms of using fetch(), ✅.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, there is an open issue about dropping IE 11 in the upcoming release: #325

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also check the behaviour of what the page looks like (is it empty? Does it have a heading but no content? I think there's an issue for this already)

#193

@pauarge pauarge merged commit cf54b1f into develop Sep 20, 2021
@pauarge pauarge deleted the remove/jquery branch September 20, 2021 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants