Closed felixarntz closed 2 years ago
@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?
@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.
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 https://github.com/google/site-kit-wp/pull/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 |
---|---|
Again, keen to hear your thoughts and happy to go with whatever you decide 👍
@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.
@felixarntz would it be possible to let us know on this one so we can push it through for Sprint 57 please?
@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 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
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.
@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.
@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?
@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).
@felixarntz not sure this would be possible with pure css alone, but we could definitely take the in-progress state of an action into account as to whether or not the buttons should be shown or hidden 👍
One suggestion I have about this (somewhat unrelated to the action itself but seems relevant here), would be to set a minimum amount of time that the spinner will show for to avoid a flicker for fast requests. Think of it like throttling interface changes. This is not really hard to do, but makes for a smoother experience for the user. What do you think?
@felixarntz Unfortunately we won't be able to use a pure CSS only solution 😞 We can add a temporary class for the wrapper containing the details for the idea and then just display the icons as we do on focus and on hover: https://github.com/google/site-kit-wp/blob/develop/assets/sass/components/idea-hub/_googlesitekit-idea-hub-dashboard-ideas-widget.scss#L168-L182
@asvinb I think what you're saying is the same what I meant :) We can dynamically toggle an extra class in JS (e.g. for as long as one of the three interactions is progressing) and if that class is there the container shows up permanently instead of only on hover/focus.
@aaemnnosttv Hmm, I'm not sure on the benefit of keeping the spinner for a minimum amount of time. Since the idea is moved out of the current tab anyway for any of the three interactions, I think the chance of an odd flickering would be quite low? Also all of them involve an API request, so there would always be some delay.
I've updated the ACs for this, let me know if this looks good.
Hmm, I'm not sure on the benefit of keeping the spinner for a minimum amount of time. Since the idea is moved out of the current tab anyway for any of the three interactions, I think the chance of an odd flickering would be quite low? Also all of them involve an API request, so there would always be some delay.
@felixarntz it's more of a concern when showing a loading state for a request to the "local" backend (WP) as this can sometimes be very fast. In our case, we have an additional request to the Idea Hub API so it's less of a concern in this instance so it's probably fine to not add extra behavior here.
ACs LGTM 👍 Sending back to IB for final touches, particularly around the last (new) AC point.
IB :heavy_check_mark:
@felixarntz Something else I noticed when working on this issue is that when we click on the create draft button, all the buttons go away and there is the "Creating draft" text will is displayed. I think since we now have individual loading icons for each type of activity, we can remove the "Creating draft" text and replace it with the loading icon to be consistent with how the other buttons behave. Let me know what you think.
@asvinb Great question. I definitely agree that we should make the user experience consistent between all 4 actions, and I think the spinner should be on all 4 of them. However, there is also the part of briefly showing a "success" message on the idea which we currently have for creating drafts but not the other 4.
With your current PR #4444 the experience is as follows:
I think we should do one of the following to make the experience consistent:
I'd vote for 1., mainly because the success indicator also allows the idea to not immediately "disappear". @marrrmarrr @samitron7 What do you think?
I think we should do one of the following to make the experience consistent:
@felixarntz I think it would be best to have an informative message for each action but I don't think it belongs in the same place it is now. IMO this is really what Material's snack bar is for. This would be similar to how Gmail works for actions on an email. One benefit of this approach would be the opportunity to allow for an undo, which we already have an issue for addressing at some point (#3913). Refactoring with a snack bar here is definitely out of scope here but I think would be good to move towards in one or more follow-up issues.
If it isn't a worse experience or UI then maybe we could add the message for all now as a first step in that direction.
@felixarntz @aaemnnosttv Just FYI, we have #3902 which relates to what we are trying to do. Maybe we can create a follow up issue which is blocked by #3902 or look into Material's snack bar.
@aaemnnosttv Yeah I agree eventually a snack bar would be best, but that goes beyond this issue. I'm open to either removing the success messages for now or adding them for all 4 actions - unless having the success messages show up right after the loading spinner is really a weird UX pattern (thoughts @marrrmarrr @samitron7?), I'd say that currently provides better insight into what's going on.
@asvinb @aaemnnosttv After discussing this with @marrrmarrr, let's go with my suggestion 1. in https://github.com/google/site-kit-wp/issues/3907#issuecomment-987039245:
@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!
@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?
@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.
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.
Verified:
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
Additional Acceptance criteria
See https://github.com/google/site-kit-wp/issues/3907#issuecomment-989000016.
Implementation Brief
assets/js/modules/idea-hub/datastore/constants.js
:IDEA_HUB_ACTIVITY_IS_DELETING
IDEA_HUB_ACTIVITY_IS_PINNING
IDEA_HUB_ACTIVITY_IS_UNPINNING
IDEA_HUB_ACTIVITY_IS_PROCESSING
assets/js/modules/idea-hub/components/dashboard/DashboardIdeasWidget/Idea.js
IDEA_HUB_ACTIVITY_IS_PROCESSING
handleDelete
,handlePin
andhandleUnpin
callbacks, replaceIDEA_HUB_ACTIVITY_IS_PROCESSING
with the corresponding activity constant, soIDEA_HUB_ACTIVITY_IS_DELETING
insidehandleDelete
etc..googlesitekit-idea-hub__idea--is-processing
class name to the.googlesitekit-idea-hub__idea--single
wrapper if there is an activity in progress.IDEA_HUB_BUTTON_DELETE
,IDEA_HUB_BUTTON_PIN
andIDEA_HUB_BUTTON_UNPIN
buttons, passCircularProgress
withsize
set to24
as prop, as the value for theicon
prop if the corresponding activity constant is truthy. So ifIDEA_HUB_ACTIVITY_IS_DELETING
is truthy, passCircularProgress
; if not, pass the appropriate existing icon.IDEA_HUB_ACTIVITY_IS_PROCESSING
to set thedisabled
attributes on the create and view buttons with a check to see ifactivity
is truthy. In other words, disable the buttons if there is anactivity
in progress.assets/sass/components/idea-hub/_googlesitekit-idea-hub-dashboard-ideas-widget.scss
,.googlesitekit-idea-hub__idea--actions svg
to exlude the.MuiCircularProgress-svg
so that we are not setting any styles on theCircularProgress
svg..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
Visual Regression Changes
QA Brief
Changelog entry