Make WordPress Core

Opened 11 years ago

Closed 7 years ago

Last modified 6 years ago

#23120 closed enhancement (fixed)

There should be indication that widget settings have been saved

Reported by: jacopovip's profile jacopo.vip Owned by: westonruter's profile westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 3.5
Component: Widgets Keywords: has-patch needs-testing dev-feedback
Focuses: ui Cc:

Description

There's currently no confirmation that adding a widget or changes to a widget have been saved. In some cases(fast hosting and a solid internet connection) the changes are saved so quickly the spinner is barely on the screen at all. I think a best course of action would be to have the word "Saved" appear briefly after the spinner goes away. Or perhaps a check mark as that doesn't need to be translated.
Related post on make.wordpress.org: http://make.wordpress.org/ui/2013/01/02/this-morning-i-ran-2-users-through-some/

Attachments (24)

23120.search-widget-settings-ru_RU.png (5.3 KB) - added by SergeyBiryukov 11 years ago.
widgetpatch.diff (1.2 KB) - added by adamsilverstein 11 years ago.
adds a checkmark after spinner on widget save & reorder
23120.patch (1.7 KB) - added by SergeyBiryukov 11 years ago.
23120.diff (2.2 KB) - added by cdog 11 years ago.
no-2x.png (2.1 KB) - added by cdog 11 years ago.
yes-2x.png (1.5 KB) - added by cdog 11 years ago.
23120.2.diff (2.2 KB) - added by adamsilverstein 11 years ago.
refresh
23120.3.diff (3.3 KB) - added by adamsilverstein 11 years ago.
uses uploader-icons for images
23120.4.diff (2.4 KB) - added by cdog 11 years ago.
spinner-icons.png (3.7 KB) - added by cdog 11 years ago.
spinner-icons-2x.png (4.5 KB) - added by cdog 11 years ago.
spinner-icons-alt.png (3.7 KB) - added by cdog 11 years ago.
spinner-icons-alt-2x.png (4.3 KB) - added by cdog 11 years ago.
23120.5.diff (2.4 KB) - added by adamsilverstein 11 years ago.
update against current
23120.6.diff (2.3 KB) - added by adamsilverstein 11 years ago.
update against trunk
23120.7.diff (2.2 KB) - added by adamsilverstein 10 years ago.
update against trunk, cleanup
23120.8.diff (2.0 KB) - added by Ipstenu 10 years ago.
Dashicon'd version of 7
23120.9.diff (2.3 KB) - added by adamsilverstein 10 years ago.
delay before fading, darker green & red
23120.10.diff (2.3 KB) - added by adamsilverstein 10 years ago.
combined widget change notifications
23120.11.diff (2.4 KB) - added by adamsilverstein 10 years ago.
use Deferred callbacks
23120.12.diff (2.5 KB) - added by adamsilverstein 10 years ago.
bigger red X on failure
23120.13.diff (3.7 KB) - added by adamsilverstein 9 years ago.
update widget save/fail notifications
23120.14.diff (3.0 KB) - added by cdog 8 years ago.
saved-button-state.png (101.5 KB) - added by westonruter 7 years ago.
Save button vs Saved button in JS Widgets plugin

Download all attachments as: .zip

Change History (78)

#1 @SergeyBiryukov
11 years ago

I think a best course of action would be to have the word "Saved" appear briefly after the spinner goes away. Or perhaps a check mark as that doesn't need to be translated.

A check mark seems better to me.

In ru_RU, there's no place for the string: 23120.search-widget-settings-ru_RU.png, so it would have the same problem as in #21890.

#2 @SergeyBiryukov
11 years ago

  • Keywords ui-focus added

#3 @adamsilverstein
11 years ago

  • Cc ADAMSILVERSTEIN@… added

#4 follow-up: @adamsilverstein
11 years ago

i have attached a patch that i believe addresses this issue

i added a class to wp-admin.css to show the yes.png checkmark bundled in /wp-admin/images

i changed the hide() action in widgets.js when the save is complete, instead i add the new class and initiate a 1 second delay() with a half second fade at the end bringing opacity to 0. finally, a callback resets the opacity to 1 and completes the hide() in the original code.

note! you can't just call delay(1500).hide() or the element gets hidden immediately (hide is not added to jquery's fx queue). the callback is required to have the hide actually happen after the delay. although the fade is not required i kinda think it looks nice.

also, i changed the spinner for dragging and resorting the widgets - it now also shows the checkmark briefly.

tested on mac chrome and firefox

Last edited 11 years ago by adamsilverstein (previous) (diff)

@adamsilverstein
11 years ago

adds a checkmark after spinner on widget save & reorder

#5 @adamsilverstein
11 years ago

  • Keywords has-patch added

#6 in reply to: ↑ 4 @jacopo.vip
11 years ago

Replying to adamsilverstein:

i have attached a patch that i believe addresses this issue

i added a class to wp-admin.css to show the yes.png checkmark bundled in /wp-admin/images

i changed the hide() action in widgets.js when the save is complete, instead i add the new class and initiate a 1 second delay() with a half second fade at the end bringing opacity to 0. finally, a callback resets the opacity to 1 and completes the hide() in the original code.

note! you can't just call delay(1500).hide() or the element gets hidden immediately (hide is not added to jquery's fx queue). the callback is required to have the hide actually happen after the delay. although the fade is not required i kinda think it looks nice.

also, i changed the spinner for dragging and resorting the widgets - it now also shows the checkmark briefly.

tested on mac chrome and firefox

I think that's a perfect solution. Good work :)

#7 @adamsilverstein
11 years ago

great, glad you like what i came up with! i'm new to contributing patches here, so thanks...

seems like there might be other places it could be used in the interface, but i haven't hunted. running locally sure makes the issue easy to spot :)

#8 @MikeHansenMe
11 years ago

  • Cc mdhansen@… added

Tested and works as described. I think it is a good solution, no text or translation needed.

#9 follow-up: @SergeyBiryukov
11 years ago

23120.patch:

  • Removes the class after hiding the element, otherwise the spinner image no longer appears on next click.
  • When saving the order after dragging widgets, only displays the spinner in sidebar header (as it is now), not in each widget.
  • Adds the widget object as a context to the selector, in order to only animate the spinner in the current widget.

#10 @SergeyBiryukov
11 years ago

  • Milestone changed from Awaiting Review to 3.6

#11 in reply to: ↑ 9 @adamsilverstein
11 years ago

Replying to SergeyBiryukov:

23120.patch:

  • Removes the class after hiding the element, otherwise the spinner image no longer appears on next click.
  • When saving the order after dragging widgets, only displays the spinner in sidebar header (as it is now), not in each widget.
  • Adds the widget object as a context to the selector, in order to only animate the spinner in the current widget.

looks good. i had added the class removal on my end, but you beat me to it. plus your patch adds the widget context to the selection which makes sense, thanks.

@cdog
11 years ago

@cdog
11 years ago

@cdog
11 years ago

#13 follow-up: @cdog
11 years ago

  • Cc catalin.dogaru@… added
  • Keywords needs-testing added

attachment:23120.diff adds two new classes, .success and .error, to use with spinners. Their use is to indicate if the request was completed succesfully or not. Personally I don't think the use .delay() is really necessary. Fading out slowly the success/error icons should be sufficient. Added also support for the HDPI versions of yes.png and no.png images.

Would be nice to merge with ticket:22839 for a better standardization of spinners.

#14 in reply to: ↑ 13 @adamsilverstein
11 years ago

Replying to cdog:

attachment:23120.diff adds two new classes, .success and .error, to use with spinners. Their use is to indicate if the request was completed succesfully or not. Personally I don't think the use .delay() is really necessary. Fading out slowly the success/error icons should be sufficient. Added also support for the HDPI versions of yes.png and no.png images.

Would be nice to merge with ticket:22839 for a better standardization of spinners.

great improvements. needs merge with #22839 and i think similar success/error code across all spinners.

@adamsilverstein
11 years ago

refresh

#15 @adamsilverstein
11 years ago

23120.2.diff

  • refresh agains trunk
  • copy attached images into wp-admin/images

#16 follow-up: @helen
11 years ago

NB: Not actually testing the patch just yet.

Those yes/no icons are ugly, don't match the admin, and make me super sad. We've got way better ones in the wp-includes/images/uploader-icons*.png sprite, although you can't go guessing at where those are located so we'd need to copy them (or put a version of them) into wp-admin/images. If the coloring doesn't seem right, JerrySarcastic should be able to help us there, as he created those icons, or anybody handy with the source, which is located in the design repo: http://design.svn.wordpress.org/production/wp-includes/images/

#17 in reply to: ↑ 16 @adamsilverstein
11 years ago

thanks for the review. my original patch referenced the existing yes.png and no.png files in wp-includes/images. the new images from @cdog are the retina versions and upon review look very similar to the original versions.

the sprite images you referenced are b&w, is that preferable to the color icons we are using now? it seems like green check and red x include color to help make the action clearer, but if they don't match the rest of the interface b&w might make more sense.

if the images are already in wp-includes/images it seems like we can reference them directly? or is there a reason to copy them over to wp-admin instead of in wp-includes? i can rework the css to use the appropriate images, just let me know what you think is best!

Last edited 11 years ago by helen (previous) (diff)

#18 follow-up: @helen
11 years ago

We can't guess at where wp-includes is located from a CSS file in wp-admin. I don't know if B&W is preferable to color in this instance, but those yes/no icons are ugly, period, and are only used in one place - insert from URL in the old media Thickbox that doesn't even appear by default anymore. If a different color is appropriate (or size), then like I said, I'm sure Jerry or another person handy with Photoshop can help out with that.

@adamsilverstein
11 years ago

uses uploader-icons for images

#19 in reply to: ↑ 18 ; follow-up: @adamsilverstein
11 years ago

Replying to helen:

We can't guess at where wp-includes is located from a CSS file in wp-admin. I don't know if B&W is preferable to color in this instance, but those yes/no icons are ugly, period, and are only used in one place - insert from URL in the old media Thickbox that doesn't even appear by default anymore. If a different color is appropriate (or size), then like I said, I'm sure Jerry or another person handy with Photoshop can help out with that.

i knew there was a reason, thanks for explaining :)

the attached 23120.3.diff​ uses the suggested uploader-icons files, to get it working just copy the images from wp-includes/images to wp-admin/images. after reviewing i like the b&w fine, no need for color really - the shape says it all. i did not get a chance to test this on a HiDPI display, but it should work according to my calculations.

@cdog
11 years ago

@cdog
11 years ago

#20 in reply to: ↑ 19 @cdog
11 years ago

Replying to helen:

I don't know if B&W is preferable to color in this instance, but those yes/no icons are ugly, period.

Agreed. They were used as placeholders while writing the patch.

Replying to adamsilverstein:

the attached 23120.3.diff​ uses the suggested uploader-icons files, to get it working just copy the images from wp-includes/images to wp-admin/images.

Thanks for keeping the patch up to date. :)

Please check: attachment:23120.4.diff. After applying the patch, the spinner-icons (grayscale) or spinner-icons-alt (color) must be placed in wp-admin/images. The icons are based on uploader-icons.psd. If the shapes and/or colors need correction, I can provide the PSD files. Personally, I prefer the grayscale version. It fits better with the existing UI, even if the color version is more suggestive.

@adamsilverstein
11 years ago

update against current

#21 @adamsilverstein
11 years ago

23120.5.diff updates 23120.4.diff against current

@adamsilverstein
11 years ago

update against trunk

#22 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

@adamsilverstein
10 years ago

update against trunk, cleanup

#23 @adamsilverstein
10 years ago

23120.7.diff: patch against trunk & a bit of cleanup/optimization

NOTE: to use this patch, you must copy uploader-icons.png and uploader-icons-2x.png from wp-includes/images to wp-admin/images.

#24 @helen
10 years ago

Can we use Dashicons for this instead?

@Ipstenu
10 years ago

Dashicon'd version of 7

#25 follow-ups: @Ipstenu
10 years ago

Added 23120.8.diff which is a Dashicon'd version.

That said I don't like how it fades out. The JS is a little beyond me right now, but wouldn't it be nicer if it just left that green checkbox up for a while.

(Colors are crap, I pulled 'em from what we use for the revisions, but they're too pale.)

#26 in reply to: ↑ 25 @adamsilverstein
10 years ago

Replying to Ipstenu:

Added 23120.8.diff which is a Dashicon'd version.

That said I don't like how it fades out. The JS is a little beyond me right now, but wouldn't it be nicer if it just left that green checkbox up for a while.

(Colors are crap, I pulled 'em from what we use for the revisions, but they're too pale.)

thanks! I will update the js and try a brief pause and quicker fade.

#27 @adamsilverstein
10 years ago

23120.9.diff - add a brief delay before fading checkmark, fade more quickly. changed color to a dark green, I'm sure there is a better color, please adjust! also removed background from success/error otherwise spinner still shows.

#28 in reply to: ↑ 25 @adamsilverstein
10 years ago

Replying to Ipstenu:

Added 23120.8.diff which is a Dashicon'd version.

That said I don't like how it fades out. The JS is a little beyond me right now, but wouldn't it be nicer if it just left that green checkbox up for a while.

(Colors are crap, I pulled 'em from what we use for the revisions, but they're too pale.)

thanks for finding those icons, that would have taken me all day :) quick note on your patch - please generate the patch from trunk (or src) for easier application, yours had the full file paths which i had to edit before applying; also your css additions had spaces not tabs.

@adamsilverstein
10 years ago

delay before fading, darker green & red

#29 follow-up: @melchoyce
10 years ago

As an FYI, Shaun Andrews has been playing around with a fun "saved" indication for widgets: https://cloudup.com/ckJmakw7HDg

#30 in reply to: ↑ 29 ; follow-ups: @adamsilverstein
10 years ago

Replying to melchoyce:

As an FYI, Shaun Andrews has been playing around with a fun "saved" indication for widgets: https://cloudup.com/ckJmakw7HDg

Very nice! I like that very much, is there a red X for failures? if we can get the patch here I would vote for that implementation over what we have here.

#31 in reply to: ↑ 30 @melchoyce
10 years ago

  • Cc mel@… added

Unsure — will ping Shaun about it.

Replying to adamsilverstein:

Very nice! I like that very much, is there a red X for failures? if we can get the patch here I would vote for that implementation over what we have here.

#32 in reply to: ↑ 30 ; follow-up: @shaunandrews
10 years ago

Replying to adamsilverstein:

Replying to melchoyce:

As an FYI, Shaun Andrews has been playing around with a fun "saved" indication for widgets: https://cloudup.com/ckJmakw7HDg

Very nice! I like that very much, is there a red X for failures? if we can get the patch here I would vote for that implementation over what we have here.

I don't have a red x for failures yet — would love some help adding that. Right now its just a simple js timer that adds and removed a class:

// Clicking save will close the widget inside
$( '#widgets-right .widgets-sortables' ).on( 'click', '.widget-control-save', function() {
	var widget = $(this).closest( '.widget' );

	var close_check = setInterval( function() {
		if ( widget.find( '.spinner' ).is( ':hidden' ) ) {
			widget.find( '.widget-title' ).click();

			setTimeout( function() {
				widget.addClass( 'saved' );
				setTimeout( function() {
					widget.removeClass( 'saved' );
				}, 3500 );
			}, 100 );
			clearInterval(close_check);
		}
	}, 100 );
} );

The CSS adds a :before element with the dashicon, which is then transitioned in/out based on the classed added by the JS:

.widget {
	position: relative;
	border-left: 0 solid transparent;
	transition: border-left 0.2s ease-in-out;
}
.widget:before {
	opacity: 0;
	display: block;
	content: "\f147";
	transition: opacity 0.2s ease-in-out;
	-webkit-font-smoothing: antialiased;
	font: normal 32px/1 'dashicons';
	position: absolute;
		top: 2px;
		left: -40px;
	line-height: 43px;
	color: #fff;
}

.widget.saved {
	border-left: 48px solid #84d24c;
}
.widget.saved:before {
	opacity: 1;
}

This code comes from the Better Widgets plugin: http://wordpress.org/plugins/better-widgets

#33 in reply to: ↑ 32 ; follow-up: @adamsilverstein
10 years ago

Thanks, I would be happy to work this up in a patch and add the failure X - is this code already part of the Widget Refresh you are working on? Should I add patch against that instead of core (I haven't tried the plugin yet, will soon)?

(Replying to shaunandrews)

#34 @helen
10 years ago

#20770 was marked as a duplicate.

#35 in reply to: ↑ 33 ; follow-ups: @shaunandrews
10 years ago

Replying to adamsilverstein:

Thanks, I would be happy to work this up in a patch and add the failure X - is this code already part of the Widget Refresh you are working on? Should I add patch against that instead of core (I haven't tried the plugin yet, will soon)?

(Replying to shaunandrews)

I think we can just patch it against core.

#36 in reply to: ↑ 35 @adamsilverstein
10 years ago

Replying to shaunandrews:

Replying to adamsilverstein:

Thanks, I would be happy to work this up in a patch and add the failure X - is this code already part of the Widget Refresh you are working on? Should I add patch against that instead of core (I haven't tried the plugin yet, will soon)?

(Replying to shaunandrews)

I think we can just patch it against core.

Roger that, will do.

@adamsilverstein
10 years ago

combined widget change notifications

#37 in reply to: ↑ 35 @adamsilverstein
10 years ago

Replying to shaunandrews:

I think we can just patch it against core.

23120.10.diff combines @shaunandrews for success notification and a red X replacing the spinner for failures from previous patches. Looks like this -

success - http://cl.ly/000B1X3b3j1C
failure - http://cl.ly/3K2T0s1N1l2d

#38 follow-ups: @melchoyce
10 years ago

When/why would saving a widget fail? Displaying just an X feels a little obscure — is there any way we can provide the user with more feedback so they can try to fix whatever went wrong?

@adamsilverstein
10 years ago

use Deferred callbacks

#39 in reply to: ↑ 38 @adamsilverstein
10 years ago

Replying to melchoyce:

When/why would saving a widget fail? Displaying just an X feels a little obscure — is there any way we can provide the user with more feedback so they can try to fix whatever went wrong?

Saving would fail if you were offline or the server was unresponsive when you clicked the save button. The goal is to provide user feedback so they are aware the save failed. Completely agree the red X is weak, what about a white X on red background matching the green success indicator?

Thinking widget shouldn't collapse when save fails, because user is going to want to try clicking save again.

#40 in reply to: ↑ 38 ; follow-up: @adamsilverstein
10 years ago

Replying to melchoyce:

When/why would saving a widget fail? Displaying just an X feels a little obscure — is there any way we can provide the user with more feedback so they can try to fix whatever went wrong?

More context would be good, but we don't really know what went wrong. What about adding the phrase 'Save failed, please try again' next to the X?

Here is what I have so far: screencast

@adamsilverstein
10 years ago

bigger red X on failure

#41 in reply to: ↑ 40 ; follow-up: @melchoyce
10 years ago

Replying to adamsilverstein:

More context would be good, but we don't really know what went wrong. What about adding the phrase 'Save failed, please try again' next to the X?

Here is what I have so far: screencast

The "save failed, try again" prompt sounds like a good compromise if we're unable to know why saving failed. That plus the white x in a red box seems like it would be a clear combination. The x looks a little off-center, but that might just be the screencast.

FYI — looks like the patch needs a small refresh since colors.css is gone.

#42 in reply to: ↑ 41 @adamsilverstein
10 years ago

Replying to melchoyce:

Replying to adamsilverstein:

More context would be good, but we don't really know what went wrong. What about adding the phrase 'Save failed, please try again' next to the X?

Here is what I have so far: screencast

The "save failed, try again" prompt sounds like a good compromise if we're unable to know why saving failed. That plus the white x in a red box seems like it would be a clear combination. The x looks a little off-center, but that might just be the screencast.

FYI — looks like the patch needs a small refresh since colors.css is gone.

Thank you for the feedback. I will add the text, check the X centering and refresh against trunk.

@adamsilverstein
9 years ago

update widget save/fail notifications

#43 @adamsilverstein
9 years ago

  • Keywords dev-feedback added

23120.13.diff:

  • refresh against trunk
  • adds wording for save failures 'Save failed, try again'

Success: https://cloudup.com/ce7eqboCFFB
Failure: https://cloudup.com/c99te--mXAq

#44 @chriscct7
8 years ago

  • Keywords needs-refresh added

@cdog
8 years ago

#45 @cdog
8 years ago

  • Keywords needs-refresh removed

Refreshed the patch and refactored the code a bit. attachment:23120.14.diff​ applies against the current revision.

@westonruter
7 years ago

Save button vs Saved button in JS Widgets plugin

#46 @westonruter
7 years ago

I just came across this issue when integrating JS Widgets into the widgets admin screen. The direction I took was analogous to what is done in the customizer (surprise), where if the widget is not saved yet then the button says “Save” as it does currently, but once the widget has been updated (and the widget-updated event triggers) then the button becomes disabled and the text changes to “Saved”. Upon making another change to the widget, then the button becomes enabled again and the text returns to “Save”. In the case of an HTTP error, then the enabled Save button just will never turn to Saved. If the WP_JS_Widget failed a validation constraint, then the button would remain “Save” but a validation notification would appear in the control.

See saved-button-state.png and corresponding plugin commit: https://github.com/xwp/wp-js-widgets/commit/878b422e4c911bf80ce067ddd2a77cf110a7338b

Last edited 7 years ago by westonruter (previous) (diff)

#47 @westonruter
7 years ago

#40813 was marked as a duplicate.

#48 @westonruter
7 years ago

  • Milestone changed from Future Release to 4.9

A patch has been added on #41610 which implements a saved indicator by simply disabling the Save button. When a widget is first added or an existing widget is opened, the Save button is initially disabled. Upon making change to field, the button is then enabled until the button is clicked and the Ajax request returns successfully. See https://core.trac.wordpress.org/ticket/41610#comment:8

#49 @westonruter
7 years ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 41352:

Widgets: Add dirty state tracking for widgets on admin screen.

  • Mark a widget as dirty when a field input triggers a change or input event; clear dirty state when widget is successfully saved.
  • Disable Save button and re-label "Saved" when widget not dirty.
  • Show AYS dialog when leaving widgets admin screen with unsaved changes.
  • When widgets are dirty, expand all unsaved widgets at AYS check and focus on first one.
  • Change "Close" link to "Done"; hide link when widget is dirty and reveal when saved.
  • The "Done" link persistently appears in the Customizer even after making a change (when the widget is dirty) because changes are autosaved into the changeset.
  • Prevent saving widget when form fails checkValidity.
  • Fix frequency of triggering of change event on the rich Text widget's textarea limited now to when there are actual changes.
  • Add a class of widget-dirty to widget containers when the widget has unsaved changes.

Props westonruter, timmydcrawford, melchoyce.
Fixes #41610, #23120.

This ticket was mentioned in Slack in #accessibility by westonruter. View the logs.


6 years ago

#51 @westonruter
6 years ago

In 41813:

Widgets: Allow deletion even when widget form fails validity checks.

Props felipeelia.
Amends [41352].
See #23120.
Fixes #42127.

#52 @westonruter
6 years ago

In 41814:

Widgets: Clear dirty flag on widgets admin screen when widget is deleted to prevent irrelevant confirmation prompt when leaving.

Props hazimayesh, felipeelia.
Amends [41352], [41813].
See #23120, #42127.
Fixes #41894.

#53 @westonruter
6 years ago

In 42521:

Widgets: Prevent checkValidity from running on a form when widget is first adding to sidebar.

Amends [41352].
See #23120.
Fixes #43003 for trunk.

#54 @westonruter
6 years ago

In 42522:

Widgets: Prevent checkValidity from running on a form when widget is first adding to sidebar.

Amends [41352].
See #23120.
Fixes #43003 for 4.9 branch.

Note: See TracTickets for help on using tickets.