WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#37510 closed enhancement (fixed)

Shiny Updates: consider to improve the bulk update failure notices

Reported by: afercia Owned by: jorbin
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: Plugins Keywords: has-screenshots has-patch commit i18n-change
Focuses: ui, accessibility, javascript Cc:

Description

When bulk-updating plugins and there's some failure in the update process, Shiny Updates displays a notice on top of the screen:

https://cldup.com/ckEBuqYS5e.png

Users can use bulk update also to update just one plugin. In this case, the error message could be improved a bit:

https://cldup.com/JGJtQYa4hW.png

To reproduce a failure message:

  • select one or more plugins to update
  • disconnect from the internet
  • select "Update" from the Bulk Actions
  • press "Apply" and wait for a few seconds

Also, since the "1 failure" is a button that expands a panel with the error details, it should use an aria-expanded attribute to be dynamically toggled to give assistive technologies users some feedback about what's happening on the screen.

https://cldup.com/GHLJmrEXqo.png

Attachments (9)

expend true.png (75.0 KB) - added by juhise 3 years ago.
expend-false.png (69.3 KB) - added by juhise 3 years ago.
37510.patch (4.1 KB) - added by juhise 3 years ago.
37510.1.patch (4.2 KB) - added by Ankit K Gupta 3 years ago.
Updated patch file
multiple-plugin.gif (232.3 KB) - added by Ankit K Gupta 3 years ago.
37510.2.patch (4.3 KB) - added by juhise 3 years ago.
updated patch
37510.3.patch (4.7 KB) - added by afercia 3 years ago.
37510.diff (4.9 KB) - added by jorbin 3 years ago.
37510.2.diff (5.4 KB) - added by afercia 3 years ago.

Download all attachments as: .zip

Change History (30)

#1 follow-ups: @swissspidy
3 years ago

  • Version set to trunk

Bulk updates were improved in Shiny Updates V2, so clearly a regression in trunk. The second screenshot indeed looks odd.

Unfortunately I don't have much time at the moment, but it would be great if we could fix this in 4.6

#2 in reply to: ↑ 1 @juhise
3 years ago

Replying to swissspidy:

Unfortunately I don't have much time at the moment, but it would be great if we could fix this in 4.6

I tried to fix this. As per @afercia suggestion, added aria-expanded attribute.
I am tester so not much good in coding. May be this needs improvement.
I have added toggle arrow, so by this user can know what's happening on the screen.

@juhise
3 years ago

@juhise
3 years ago

@juhise
3 years ago

#3 @juhise
3 years ago

  • Keywords has-patch added; needs-patch removed

#4 @Ankit K Gupta
3 years ago

@juhise
Seems you missed the case when more than one failure appears. I have added this case in your code.
Thanks

Last edited 3 years ago by Ankit K Gupta (previous) (diff)

@Ankit K Gupta
3 years ago

Updated patch file

#5 @Ankit K Gupta
3 years ago

  • Keywords reporter-feedback added

#6 @afercia
3 years ago

@juhise @Ankit K Gupta thanks very much the approach looks good. I'd recommend a few things:

  • the %s failure (singular and plural) string should be improved a bit; to keep it simple, maybe there's no need to distinguish between plugins and themes
  • this should be translatable: <span class="screen-reader-text">Toggle panel: Failure Notice</span>
  • the patch should use some coding standards, mainly about braces and spaces; also, WordPress uses tabs for indentation and not spaces, this has probbly something to do with some settings in the editor you're using
  • the if/else to toggle the aria-expanded value maybe could be avoided and use just the value of $bulkActionNotice.find( 'ul' ).hasClass( 'hidden' ) which already returns true or false
  • jQuery optimisation: instead of getting the same elements multiple times, I'd get them once and store them in a variable for later reuse

#7 @afercia
3 years ago

  • Keywords reporter-feedback removed

@juhise
3 years ago

updated patch

#8 @juhise
3 years ago

Tried to update the patch according to the suggestion.

#9 @juhise
3 years ago

  • Keywords needs-testing added

#10 in reply to: ↑ 1 @juhise
3 years ago

Replying to swissspidy:

Unfortunately I don't have much time at the moment, but it would be great if we could fix this in 4.6

@swissspidy
Are we not going to fix this in 4.6?

This ticket was mentioned in Slack in #feature-shinyupdates by swissspidy. View the logs.


3 years ago

@afercia
3 years ago

#12 @afercia
3 years ago

  • Keywords commit added; needs-testing removed

Refreshed a bit the patch, looks good to me.

  • changed the "failure/s" strings
  • added a couple CSS classes to make things easier
  • removed a few unnecessary things

In updates.js the patch removes also some tab characters (my editor does that automatically) probably previous developments leftovers, so there will be 5 lines with unrelated changes.

If it's going to make it in 4.6, then it should have the i18n-change keyword.

This ticket was mentioned in Slack in #feature-shinyupdates by afercia. View the logs.


3 years ago

@jorbin
3 years ago

#14 @jorbin
3 years ago

  • Owner set to ocean90
  • Status changed from new to assigned

I've updated the patch to use a class and target it for the css. I kept the 'Toggle panel: Failure Notice' string as we use a similar string already in https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/template.php#L1039

#15 follow-up: @ocean90
3 years ago

  • Keywords shiny-updates commit removed

37510.diff doesn't work because there is no such CSS class, 37510.3.patch works as expected.


I kept the 'Toggle panel: Failure Notice' string as we use a similar string already

Yep, but "toggle" and "panel" are words which are not easy translatable in some languages. Can we use "Show more details"?

#16 in reply to: ↑ 15 @afercia
3 years ago

Replying to ocean90:

37510.diff doesn't work because there is no such CSS class

Looking, I guess it just needs {{ data.className }} to be added in the template.

"toggle" and "panel" are words which are not easy translatable in some languages. Can we use "Show more details"?

"Toggle" is handy because, for example, you don't need to dynamically update "Show details" in "Hide details" and vice-versa but yeah it's difficult to translate in many languages, Italian too. Considering the whole string, and how it gets announced by screen readers thanks to the aria-expanded attribute, I'd say it's OK. See screenshots below:

https://cldup.com/6Q1YFFDzOE.png

https://cldup.com/NbGI6PeJNO.png

@afercia
3 years ago

#17 @afercia
3 years ago

  • Keywords commit added

Refreshed patch to address the last 2 issues.

#18 @ocean90
3 years ago

  • Keywords i18n-change added
  • Milestone changed from Future Release to 4.6

#19 @ocean90
3 years ago

  • Owner changed from ocean90 to jorbin
  • Status changed from assigned to reviewing

37510.2.diff looks good to me. @jorbin, what you you think?

#20 @jorbin
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 38185:

Updates: Improve bulk update failure notice

When doing a bulk update, if there are failures the user needs to know about that. This makes it clearer that you can click on the notification to see more details, especially for screen reader users.

Fixes #37510.
Props juhise, Ankit K Gupta, afercia, jorbin, ocean90. 

#21 @jorbin
3 years ago

In 38187:

Updates: Improve bulk update failure notice

Merge of [38185] to the 4.6 branch

When doing a bulk update, if there are failures the user needs to know about that. This makes it clearer that you can click on the notification to see more details, especially for screen reader users.

Fixes #37510.
Props juhise, Ankit K Gupta, afercia, jorbin, ocean90. 

Note: See TracTickets for help on using tickets.