Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Simplifies Ajax on both the JS and PHP sides. #103

Merged
merged 4 commits into from
May 6, 2020
Merged

Conversation

pbiron
Copy link
Contributor

@pbiron pbiron commented May 4, 2020

Fixes #102, #94

This simplifies Ajax:

  1. on the PHP side, combines both enable/disable handlers into a single toggle handler
  2. on the JS side, reduces it down to a single click handler
    • also removes all HTML injection. that is, the markup for all "states" is output from PHP when the screen is originally rendered and the JS basically just shows/hides the relevant parts depending on whether it is enabling or disabling

It also addresses the "unexpected/quite annoying jump/shift" when the Ajax is processing, as reported in #94. It does that with the proposed solution in #94 (comment)

Also fixes several other WPCS-related issues and an a11y problem where @aria-label wasn't correct in one place.

If this gets accepted, then we can close #99.

Also fixes several other WPCS-related issues.
@pbiron pbiron added enhancement New feature or request a11y Accessibility Core Merge Prerequisite This code that will require an update before merging to core design labels May 4, 2020
@pbiron pbiron added this to the 0.6.1 milestone May 4, 2020
@pbiron pbiron requested review from azaozz, whyisjake and audrasjb May 4, 2020 23:00
@pbiron pbiron self-assigned this May 4, 2020
@pbiron
Copy link
Contributor Author

pbiron commented May 4, 2020

While this is ready to be reviewed, it's not quite ready to be merged. It still needs at least 2 things:

  1. delay showing the spinning icon for a short time, so that if the Ajax completes really quickly would be less "jumpiness", as described in Fix/enhance the UI around the link to enable/disable auto-updates #94 (comment)
  2. remove the reliance on wpAjax.unserialize()

I've got some other things to attend to, I'll get to those as soon as I can.

@pbiron
Copy link
Contributor Author

pbiron commented May 4, 2020

@audrasjb Can you please review the way I did the strings in wp_autoupdates_toggle_auto_updates()? I not sure they are done in a way that is helpful to translators.

@pbiron pbiron mentioned this pull request May 5, 2020
js/wp-autoupdates.js Outdated Show resolved Hide resolved
}

wp.a11y.speak( 'enable' === action ? wp_autoupdates.enabled : wp_autoupdates.disabled, 'polite' );
Copy link

Choose a reason for hiding this comment

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

polite is redundant, as it's the default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@audrasjb What are your thoughts here?

Personally, I think it is sometimes useful to be explicit with params, even when using the default value...and think this is one of those cases.

} else {
parent.find( '.auto-updates-error' ).removeClass( 'hidden' ).addClass( 'notice error' ).find( 'p' ).text( response.data.error );
wp.a11y.speak( response.data.error, 'polite' );
Copy link

Choose a reason for hiding this comment

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

polite is redundant, as it's the default value.

.fail( function( response ) {
parent.find( '.auto-updates-error' ).removeClass( 'hidden' ).addClass( 'notice error' ).find( 'p' ).text( wp_autoupdates.auto_update_error );
wp.a11y.speak( wp_autoupdates.auto_update_error, 'polite' );
Copy link

Choose a reason for hiding this comment

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

polite is redundant, as it's the default value.

js/wp-autoupdates.js Outdated Show resolved Hide resolved
Copy link
Contributor

@azaozz azaozz left a comment

Choose a reason for hiding this comment

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

Thinking this is ready to merge with couple of tiny changes/improvements. Can be refined more while or after the core patch if needed.

@pbiron
Copy link
Contributor Author

pbiron commented May 5, 2020

Thinking this is ready to merge with couple of tiny changes/improvements. Can be refined more while or after the core patch if needed.

I'm just about done with replacing wpAjax.unserialize() with @data attributes. Will push that shortly.

…JS with @DaTa attributes.

Also corrects a few more coding standards issues.
@pbiron
Copy link
Contributor Author

pbiron commented May 5, 2020

@azaozz I think it can be merged now, and any other changes that are necessary can be done in a separate PR.

@pbiron pbiron requested a review from azaozz May 5, 2020 23:17
pbiron added 2 commits May 5, 2020 19:24
…f data(). Also moves the .update-link click handler to the JS file instead loading that script inline.
@pbiron pbiron merged commit 0a47e44 into master May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y Accessibility Core Merge Prerequisite This code that will require an update before merging to core design enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify Ajax
3 participants