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

Show spinner when saving, unsaving, or dismissing an idea in Idea Hub widget #3907

Closed
felixarntz opened this issue Aug 18, 2021 · 27 comments
Closed
Labels
Module: Idea Hub Google Idea Hub module related issues P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature UX Issues that require UX input

Comments

@felixarntz
Copy link
Member

felixarntz commented Aug 18, 2021

There can be a delay when clicking one of the Idea Hub widget icon buttons to save, unsave, or dismiss an idea. This delay is expected due to sending the API request, however we should cater for this in the UI by showing a loading indicator.

See https://app.asana.com/0/1200491083500938/1200758448284506/f


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • When clicking on one of the icon buttons to "Save", "Dismiss" (both in the "New" tab) or "Unsave" (in the "Saved" tab) an idea, the respective icon should be replaced by a loading spinner until the action has been completed.
  • Whenever that spinner is displayed, the "overlay" with the idea interaction buttons for that respective idea should be "permanently" displayed as long as the spinner is there / as long as the process/API request takes.
    • In other words, when hovering/focusing away from the idea while one of the interactions is in progress, the overlay shouldn't disappear like it usually does when there's no action in progress. This can be accomplished for example by conditionally applying an extra CSS class on the overlay wrapper.

Additional Acceptance criteria

See #3907 (comment).

  • All 4 actions should have the loading spinner.
  • All 4 actions should have a brief success state where they show a brief message that remains there for a brief period before the idea disappears (from the current tab) - basically this should work the same way it already does for creating a draft.
  • The success messages should be:
    • Idea saved
    • Idea dismissed
    • Idea removed from saved
    • Draft created (already exists)

Implementation Brief

  • Inside assets/js/modules/idea-hub/datastore/constants.js:
    • Add the following new activity constants:
    • IDEA_HUB_ACTIVITY_IS_DELETING
    • IDEA_HUB_ACTIVITY_IS_PINNING
    • IDEA_HUB_ACTIVITY_IS_UNPINNING
    • Remove IDEA_HUB_ACTIVITY_IS_PROCESSING
  • Inside assets/js/modules/idea-hub/components/dashboard/DashboardIdeasWidget/Idea.js
    • Import the new constants, and remove the import of IDEA_HUB_ACTIVITY_IS_PROCESSING
    • Inside the handleDelete, handlePin and handleUnpin callbacks, replace IDEA_HUB_ACTIVITY_IS_PROCESSING with the corresponding activity constant, so IDEA_HUB_ACTIVITY_IS_DELETING inside handleDelete etc.
    • Add the .googlesitekit-idea-hub__idea--is-processing class name to the .googlesitekit-idea-hub__idea--single wrapper if there is an activity in progress.
    • For each of the IDEA_HUB_BUTTON_DELETE, IDEA_HUB_BUTTON_PIN and IDEA_HUB_BUTTON_UNPIN buttons, pass CircularProgress with size set to 24 as prop, as the value for the icon prop if the corresponding activity constant is truthy. So if IDEA_HUB_ACTIVITY_IS_DELETING is truthy, pass CircularProgress; if not, pass the appropriate existing icon.
    • Replace the remaining logic that uses IDEA_HUB_ACTIVITY_IS_PROCESSING to set the disabled attributes on the create and view buttons with a check to see if activity is truthy. In other words, disable the buttons if there is an activity in progress.
  • Using assets/sass/components/idea-hub/_googlesitekit-idea-hub-dashboard-ideas-widget.scss,
    • Update .googlesitekit-idea-hub__idea--actions svg to exlude the .MuiCircularProgress-svg so that we are not setting any styles on the CircularProgress svg.
    • Add styles for .googlesitekit-idea-hub__idea--is-processing which are similar to .googlesitekit-idea-hub__idea--single:hover and ..googlesitekit-idea-hub__idea--single:focus where the icons are visible except the background color is white.

Test Coverage

  • No new tests should be required.

Visual Regression Changes

  • VRT images dealing with the DashboardIdeaWidget UI might need updating.

QA Brief

  • Setup the Idea Hub module.
  • Upon interacting with an idea, i.e pinning, unpinning, deleting or creating an idea, there should be a loading icon when activity is in progress.

Changelog entry

  • Update the Idea Hub widget to display a spinner when saving, unsaving or dismissing an idea.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature Module: Idea Hub Google Idea Hub module related issues labels Aug 18, 2021
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Aug 18, 2021
@felixarntz felixarntz self-assigned this Aug 23, 2021
@felixarntz
Copy link
Member Author

@johnPhillips The IB is pretty much good to go, just one question:

Note that you may need to tweak the CSS across several breakpoints, bearing in mind that the action buttons only appear on hover on wider viewports. For example there might be some jank on larger viewports when clicking a button and then un-hovering.

Can you clarify your thoughts on what should possibly change here? I know you discussed with @asvinb, I'd like to see a few more of those thoughts shared here. Is it a problem having the loading indicators only on hover? I'm thinking, if the user clicks one of those buttons, they're already hovering over it anyway, so they should definitely see the state change to a loading indicator. They may afterwards hover off while it's still loading, but I wonder how much this is actually a problem. If you think it is a problem, how should it be fixed?

@asvinb
Copy link
Collaborator

asvinb commented Aug 24, 2021

@johnPhillips Actually we can solve the issue about the hover state by adding another class name to the container having googlesitekit-idea-hub__idea--single as class, for e.g googlesitekit-idea-hub__idea--is-processing when activity is not null, and in the corresponding styles, we make sure the buttons are displayed irrespective of breakpoints and hover state. So when googlesitekit-idea-hub__idea--is-processing is present, the idea has the same styles as upon hover. Let me know what you think.

@johnPhillips
Copy link
Contributor

Can you clarify your thoughts on what should possibly change here?

@felix sure. To be honest, I was slightly vague there because I wasn't sure how the precise implementation would play out with #3920, which is obviously touching the same UI. I think things will have to be slightly fluid between the two issues, or we need to make the other a blocker, solve it and then come back to this one perhaps?

I know you discussed with @asvinb, I'd like to see a few more of those thoughts shared here. Is it a problem having the loading indicators only on hover?

I've outlined my thoughts over on that issue - keen to hear your feedback 👍

I'm thinking, if the user clicks one of those buttons, they're already hovering over it anyway, so they should definitely see the state change to a loading indicator. They may afterwards hover off while it's still loading, but I wonder how much this is actually a problem. If you think it is a problem, how should it be fixed?

I personally felt that it might be slightly confusing for the spinner to disappear when you mouse out because you lose that feedback that something is in play, and then the idea just disappears. I discussed with @aaemnnosttv the idea of having some sort of transient notice that appears for a few seconds to confirm that the action was successful for example, so we could look at that.

However, as regards spinners and hover states / line wrapping, I knocked together some examples:

No hover state (actions always visible, text wraps consistently) Actions appear on hover, but we retain the spinner on mouse out
spinner spinner 2

Again, keen to hear your thoughts and happy to go with whatever you decide 👍

@johnPhillips
Copy link
Contributor

@asvinb - nice idea, thank you. It might make things feel a bit more consistent, and I can add it to the IB if there is consensus. Over to you @felixarntz - let me know what your take is on all this.

@fhollis fhollis added this to the Sprint 57 milestone Aug 25, 2021
@eclarke1
Copy link
Collaborator

@felixarntz would it be possible to let us know on this one so we can push it through for Sprint 57 please?

@felixarntz
Copy link
Member Author

@eclarke1 This is pending the solution for #3839, which we've now defined in the PR conversation (that issue due to that complexity may go notably above original estimate and probably is already). I think we should hold off this one until the path of #3839 is clear. It's not a launch blocker - still, let's try again for Sprint 58 (which would likely still be in time for launch).

@alex-moulin What are your suggestions for how a loading spinner should be surfaced here? Now that the three actions will be within the three-dots menu, it changes the UI circumstances here. Should the menu close on click and the loading spinner should show somewhere else on the idea? Or should the menu remain open and the loading spinner shows within the menu next to the action you clicked? cc @johnPhillips

@felixarntz felixarntz removed this from the Sprint 57 milestone Aug 31, 2021
@eclarke1 eclarke1 added the UX Issues that require UX input label Sep 1, 2021
@alex-moulin
Copy link

@felixarntz My suggestion would be that the menu remains open and the loading spinner would show within the menu in the place of the action clicked, and the result would then either be to see the pin icon in its selected state or that the idea would disappear if it was dismissed.
Alternatively, I would only close the menu and show the spinner somewhere else if we were able to use the snackbar to confirm the action to the users once completed.

Let me know if you would rather have me mock this up.
cc. @johnPhillips @eclarke1

@felixarntz
Copy link
Member Author

Update here: It looks like we're circling back to the original approach for #3839, likely including removal of the hover state. As such, that should drastically simplify this issue as well. I'm keeping this in ACs for now though until the course of the other issue is clear.

@felixarntz felixarntz assigned marrrmarrr and unassigned felixarntz Sep 1, 2021
@felixarntz
Copy link
Member Author

@marrrmarrr @samitron7 This is now the issue where we need to make a decision on whether to remove the hover state and show the icons at all time. The hover state is problematic for the loading indicator since then they would also only show when hovering, which would be a strange experience.

@marrrmarrr
Copy link
Collaborator

@felixarntz I think from the user perspective, there's a distinction b/w when they're viewing the widget in general and when they want to complete an action.
In the first case, they don't really expect to see all action icons for each idea all the time, whereas in the second case they would expect to keep seeing them until the action is completed. Is it possible for the icons to appear on hover, but once the user clicks something, they remain displayed until the action is completed?

@felixarntz
Copy link
Member Author

@marrrmarrr

Is it possible for the icons to appear on hover, but once the user clicks something, they remain displayed until the action is completed?

This may be doable indeed, but not 100% certain based on how the widget hover state currently works. Paging our CSS guru @asvinb, do you think the above would be possible? E.g. add a class when the user interacts with one of the buttons to temporarily keep them visible (instead of only on hover).

@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Dec 10, 2021
@aaemnnosttv aaemnnosttv assigned asvinb and unassigned aaemnnosttv Dec 10, 2021
@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Dec 13, 2021
@aaemnnosttv aaemnnosttv assigned asvinb and unassigned aaemnnosttv Dec 14, 2021
@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Dec 15, 2021
@aaemnnosttv aaemnnosttv removed their assignment Dec 17, 2021
@aaemnnosttv
Copy link
Collaborator

@felixarntz / @eugene-manuilov could you give this a final look over? I was involved in some of the changes at the end and it's somewhat of a non-trivial PR. Thanks!

@asvinb asvinb assigned eugene-manuilov and unassigned asvinb Dec 21, 2021
@asvinb asvinb assigned eugene-manuilov and unassigned asvinb Dec 22, 2021
@eugene-manuilov eugene-manuilov removed their assignment Dec 22, 2021
@FlicHollis FlicHollis added the Rollover Issues which role over to the next sprint label Jan 4, 2022
@wpdarren wpdarren self-assigned this Jan 4, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Jan 4, 2022

QA Update: ⚠️

@felixarntz @eugene-manuilov when you click any of the icons, the spinner appears, but the tooltip still appears, even when you hover away. I realise this has always happened, i.e. when creating a draft, but it's odd behaviour, in my opinion. Thoughts?

idh.mp4

@eugene-manuilov
Copy link
Collaborator

@wpdarren, yes, maybe, not critical though. Create a ticket for it if you want since tooltips are out of scope for this ticket, I think.

@aaemnnosttv
Copy link
Collaborator

I think it's expected behavior for now. I'm not sure what the best solution is here but I think the tooltip offers useful information while the action is in progress. So maybe something we can enhance but, I wouldn't say it's a poor experience.

@wpdarren
Copy link
Collaborator

wpdarren commented Jan 5, 2022

QA Update: ✅

Verified:

  • Upon interacting with an idea, i.e pinning, unpinning, deleting and creating an idea, there is loading icon when in progress.
  • Tested this change with current and unified dashboard.
  • Tested this change on mobile and desktop.
idh.mp4

@wpdarren wpdarren removed their assignment Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Idea Hub Google Idea Hub module related issues P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature UX Issues that require UX input
Projects
None yet
Development

No branches or pull requests