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

Replace jQuery.extend with Object.assign #291

Merged
merged 3 commits into from
Nov 29, 2018
Merged

Conversation

OleVik
Copy link
Contributor

@OleVik OleVik commented Nov 9, 2018

Support is virtually full for all browsers (ref), if needed a polyfill could be thrown in. Results should be equal with jQuery (test). Fixes #290.

Support is virtually full for all browsers, if needed a polyfill could be thrown in. Results should be equal with jQuery. Fixes getgrav#290.
@rhukster rhukster requested a review from w00fz November 12, 2018 03:57
Copy link
Member

@w00fz w00fz left a comment

Choose a reason for hiding this comment

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

I’m ok with the change but would definitely prefer to see the polyfill in place. I’ve seen issues still with IE Edge and with this being also frontend plugin it’s probably best to be safer than sorry.
I also think this is not the only place where we do extend in the plugin, was it?

@OleVik
Copy link
Contributor Author

OleVik commented Nov 23, 2018

The only other place jQuery.extend is used is in the file-field, which requires jQuery outright. Could potentially be replaced with vanilla JS too, but this PR only backfills in where jQuery isn't really needed. I'll look at adding in the polyfill, which should resolve some IE issues if I'm not mistaken.

Needs to be recompiled into form.min.js.
@OleVik
Copy link
Contributor Author

OleVik commented Nov 24, 2018

Latest commit adds the polyfill, though as noted it needs to be compiled. The webpack and gulp setups won't run on my Windows DevEnv, but with the polyfill included (but not jQuery), it throws no errors in MS Edge.

@drzraf
Copy link
Contributor

drzraf commented Nov 28, 2018

See #305
But even then, I have:
Error: webpack.optimize.UglifyJsPlugin has been removed, please use config.optimization.minimize instead.

But running webpack directly works:
NODE_ENV=production ./node_modules/.bin/webpack --config webpack.conf.js --mode production

drzraf pushed a commit to drzraf/grav-plugin-form that referenced this pull request Nov 28, 2018
drzraf pushed a commit to drzraf/grav-plugin-form that referenced this pull request Nov 28, 2018
@w00fz
Copy link
Member

w00fz commented Nov 28, 2018

This looks good now @OleVik but where are you loading the polyfill from?

@w00fz
Copy link
Member

w00fz commented Nov 28, 2018

@OleVik Actually I missed your message, I'll look at adding it in the mix and transpiling.

Cheers

…ow language is merged so it’s not using jQuery
@w00fz
Copy link
Member

w00fz commented Nov 28, 2018

@OleVik @drzraf I am now loading the polyfill for IE only, can you guys give this a try? 10bd598

@w00fz w00fz merged commit 5cb6030 into getgrav:develop Nov 29, 2018
@OleVik OleVik deleted the patch-1 branch December 16, 2018 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants