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

Meta Boxes: Fix Dirty Detection in Safari #3934

Merged
merged 1 commit into from Dec 13, 2017

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Dec 12, 2017

closes #3929

Dirty Detection of Meta Boxes forms was broken in Safari because it was relying on FormData entriesAPI which is not available in all browsers yet. This PR uses jQuery.serialize as a cross-browser alternative.

Testing instructions

  • Install Gutenberg and Seo Press
  • Open Gutenberg
  • It should not break
const data = new window.FormData( this.form );
const entries = Array.from( data.entries() );
return entries;
return jQuery( this.form ).serialize();

This comment has been minimized.

@gziolo

gziolo Dec 12, 2017

Member

It returns a string in here after changes got applied.

getFormData is compared against this.originalFormData which is initialized as an empty array.

this.originalFormData = [];
...
const newIsDirty = ! isEqual( this.originalFormData, this.getFormData() );
@gziolo

gziolo Dec 12, 2017

Member

It returns a string in here after changes got applied.

getFormData is compared against this.originalFormData which is initialized as an empty array.

this.originalFormData = [];
...
const newIsDirty = ! isEqual( this.originalFormData, this.getFormData() );

This comment has been minimized.

@youknowriad

youknowriad Dec 12, 2017

Contributor

Good catch, changing the initial value :)

@youknowriad

youknowriad Dec 12, 2017

Contributor

Good catch, changing the initial value :)

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 12, 2017

Member

We are polyfilling fetch, but apparently we don't do it for FormData. We use FormData also for media upload:

const data = new window.FormData();
. We should test if it works in Safari, too.

Is it possible to polyfil FormData instead of using jQuery?

Member

gziolo commented Dec 12, 2017

We are polyfilling fetch, but apparently we don't do it for FormData. We use FormData also for media upload:

const data = new window.FormData();
. We should test if it works in Safari, too.

Is it possible to polyfil FormData instead of using jQuery?

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 12, 2017

Member

It looks like this polyfill would work: https://github.com/jimmywarting/FormData/blob/master/FormData.js.

Safari 11 has FormData, but has only append method is implemented.

Member

gziolo commented Dec 12, 2017

It looks like this polyfill would work: https://github.com/jimmywarting/FormData/blob/master/FormData.js.

Safari 11 has FormData, but has only append method is implemented.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 12, 2017

Member

It looks like jQuery is loaded anyways in Gutenberg. It uses a different version than the one specified in package.json: v1.12.4. Should we use the same version?
It would be nice to avoid using jQuery at all, but in the context of meta boxes it seems like must have ...

Do we need to load all the code for meta boxes every time Gutenberg loads or is there case where we could skip it? I'm thinking if it would be a good idea to put meta boxes into their own namespace and load all require code only when it's really required.

Member

gziolo commented Dec 12, 2017

It looks like jQuery is loaded anyways in Gutenberg. It uses a different version than the one specified in package.json: v1.12.4. Should we use the same version?
It would be nice to avoid using jQuery at all, but in the context of meta boxes it seems like must have ...

Do we need to load all the code for meta boxes every time Gutenberg loads or is there case where we could skip it? I'm thinking if it would be a good idea to put meta boxes into their own namespace and load all require code only when it's really required.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 12, 2017

Contributor

It looks like jQuery is loaded anyways in Gutenberg. It uses a different version than the one specified in package.json: v1.12.4. Should we use the same version?

This one is only for testing purpose, it's not even run :)

Contributor

youknowriad commented Dec 12, 2017

It looks like jQuery is loaded anyways in Gutenberg. It uses a different version than the one specified in package.json: v1.12.4. Should we use the same version?

This one is only for testing purpose, it's not even run :)

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 12, 2017

Contributor

Do we need to load all the code for meta boxes every time Gutenberg loads or is there case where we could skip it?

It small enough IMO. I don't it's worth a separate script.

Contributor

youknowriad commented Dec 12, 2017

Do we need to load all the code for meta boxes every time Gutenberg loads or is there case where we could skip it?

It small enough IMO. I don't it's worth a separate script.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 12, 2017

Member

Feel free to merge if you find someone to test it before I get to it 👍

Member

gziolo commented Dec 12, 2017

Feel free to merge if you find someone to test it before I get to it 👍

@gziolo

gziolo approved these changes Dec 13, 2017

I can confirm it works in Safari with SEOPress plugin.

@youknowriad youknowriad merged commit 55c5591 into master Dec 13, 2017

3 checks passed

codecov/project 38.23% (-0.01%) compared to 360467b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/metabox-dirty-checking branch Dec 13, 2017

@iseulde iseulde added this to the 1.9.1 milestone Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment