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

Wrap icons when there is not enough space. #3920

Merged
merged 3 commits into from
Sep 1, 2021

Conversation

asvinb
Copy link
Collaborator

@asvinb asvinb commented Aug 23, 2021

Summary

Addresses issue #3839

Relevant technical choices

Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 4.7 and PHP 5.6.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have added a QA Brief on the issue linked above.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

@google-cla google-cla bot added the cla: yes label Aug 23, 2021
@github-actions
Copy link

github-actions bot commented Aug 23, 2021

Size Change: -45 B (0%)

Total Size: 1.13 MB

Filename Size Change
./dist/assets/css/admin.css 38.5 kB -6 B (0%)
./dist/assets/js/googlesitekit-modules-idea-hub.********************.js 23.1 kB -39 B (0%)
ℹ️ View Unchanged
Filename Size
./dist/assets/css/adminbar.css 8 kB
./dist/assets/css/wpdashboard.css 4.58 kB
./dist/assets/js/33.********************.js 3.12 kB
./dist/assets/js/analytics-advanced-tracking.js 769 B
./dist/assets/js/googlesitekit-activation.********************.js 37.7 kB
./dist/assets/js/googlesitekit-adminbar.********************.js 30.6 kB
./dist/assets/js/googlesitekit-api.********************.js 9.02 kB
./dist/assets/js/googlesitekit-base.********************.js 1.59 kB
./dist/assets/js/googlesitekit-dashboard-details.********************.js 44.1 kB
./dist/assets/js/googlesitekit-dashboard-splash.********************.js 51.8 kB
./dist/assets/js/googlesitekit-dashboard.********************.js 44.1 kB
./dist/assets/js/googlesitekit-data.********************.js 1.65 kB
./dist/assets/js/googlesitekit-datastore-forms.********************.js 8.72 kB
./dist/assets/js/googlesitekit-datastore-location.********************.js 2.03 kB
./dist/assets/js/googlesitekit-datastore-site.********************.js 13.1 kB
./dist/assets/js/googlesitekit-datastore-ui.********************.js 8.69 kB
./dist/assets/js/googlesitekit-datastore-user.********************.js 20.8 kB
./dist/assets/js/googlesitekit-i18n.js 3.92 kB
./dist/assets/js/googlesitekit-idea-hub-notice.********************.js 2.4 kB
./dist/assets/js/googlesitekit-idea-hub-post-list.********************.js 23.8 kB
./dist/assets/js/googlesitekit-module.********************.js 43.5 kB
./dist/assets/js/googlesitekit-modules-adsense.********************.js 44.5 kB
./dist/assets/js/googlesitekit-modules-analytics-4.********************.js 18.8 kB
./dist/assets/js/googlesitekit-modules-analytics.********************.js 65.4 kB
./dist/assets/js/googlesitekit-modules-optimize.********************.js 18.3 kB
./dist/assets/js/googlesitekit-modules-pagespeed-insights.********************.js 16.3 kB
./dist/assets/js/googlesitekit-modules-search-console.********************.js 28.4 kB
./dist/assets/js/googlesitekit-modules-subscribe-with-google.********************.js 16.4 kB
./dist/assets/js/googlesitekit-modules-tagmanager.********************.js 29.8 kB
./dist/assets/js/googlesitekit-modules.********************.js 16.4 kB
./dist/assets/js/googlesitekit-polyfills.********************.js 376 B
./dist/assets/js/googlesitekit-settings.********************.js 48.5 kB
./dist/assets/js/googlesitekit-user-input.********************.js 44.8 kB
./dist/assets/js/googlesitekit-vendor.********************.js 312 kB
./dist/assets/js/googlesitekit-widgets.********************.js 11.4 kB
./dist/assets/js/googlesitekit-wp-dashboard.********************.js 32.5 kB
./dist/assets/js/runtime.********************.js 1.19 kB

compressed-size-action

@asvinb
Copy link
Collaborator Author

asvinb commented Aug 23, 2021

@johnPhillips I know you worked on the IB, but can you check my notes in the "Relevant technical choices" of this PR?
Thanks!

@johnPhillips
Copy link
Contributor

Thanks @asvinb - you helped me a lot with the IB, so you are in a good position to evaluate the best approach here.

It sounds good to me, but I'll mention two things I noticed when sketching in the IB we arrived at.

  1. When the idea content is verbose and the actions are not stacked, then the text has to wrap on hover, which is slightly janky:

Screenshot 2021-08-23 at 13 21 09

  1. Although at 600 to 960px there does seem a lot of space, again bear in mind that there is no character limit on these ideas, so they can get quite long:

Screenshot 2021-08-23 at 13 20 33

So here we could have gone inline with the first two items, but the third might have looked a little strange.

In summary, I'd just suggest eyeballing your solution with some verbose ideas and seeing how it looks? Happy to go with whatever you decide 👍

@asvinb
Copy link
Collaborator Author

asvinb commented Aug 23, 2021

Thanks @johnPhillips . Based on our conversation, I've tweaked the PR so that items are stacked when the viewport is < 1280px.
@aaemnnosttv @felixarntz Is there a UX reason why we are showing the icons upon hover? I'am thinking whether it'll be best to show the icons across all breakpoints, especially we have #3907 where we are replacing icons by the loading spinner? 🤔

@felixarntz
Copy link
Member

@asvinb

Is there a UX reason why we are showing the icons upon hover? I'am thinking whether it'll be best to show the icons across all breakpoints, especially we have #3907 where we are replacing icons by the loading spinner? 🤔

I believe this was decided to not visually overload the user with too many action buttons. Any additional thoughts @marrrmarrr?

@johnPhillips
Copy link
Contributor

If it is the case that we don't want to visually overload the UI with buttons, that's reasonable. But I would point out that we are showing them all on smaller viewports where there is less space:

Screenshot 2021-08-24 at 10 23 58

This is my first issue with using :hover to hide the buttons. My second is that it increases complexity for us, especially as the buttons are also sometimes inline and sometimes wrapped. For example:

  • How do we treat the spinner behaviour on a button that is sometimes hidden unless your hover over it?
  • What about verbose idea text that will have to wrap when you hover and expose the buttons (see my comment above)?

Just wondering if there is a more consistent way to keep the buttons visible but to lessen their visual impact / reduce the clutter. Maybe take a page out of WordPress's book? On the Posts page they use checkboxes and a single row of buttons that allows batch operations. They are also dealing with potentially verbose text, so they put all the actions underneath and only expose them on hover:

Screenshot 2021-08-24 at 12 13 03

Batch operations are obviously a fair amount of work. But if we always wrapped the buttons and made them visible, it could solve some of the issues I alluded to above. We could also look at secondary colours for some of the buttons to mute them slightly...

Verbose content, small viewport Large viewport
Screenshot 2021-08-24 at 12 27 21 Screenshot 2021-08-24 at 12 28 14

I'm no UX expert and I'm certainly not a designer, so these might be terrible ideas 😂. 🤷‍♂️. All I'm saying is that perhaps we can find a better way to solve the clutter concern across all viewports?

@felixarntz
Copy link
Member

felixarntz commented Aug 24, 2021

@marrrmarrr @alex-moulin Can you share your thoughts on @johnPhillips's feedback above? I tend to agree only showing the buttons on hover is causing issues here, and I'd be okay to change that.

@alex-moulin
Copy link

@felixarntz @marrrmarrr @johnPhillips I would be keen to discuss this as I don't know for sure the reasons behind some of the decisions that were taken to design these elements. However, my 2 cents would be that instead of showing all the icons by default, we could potentially show 3 vertical dots on hover, and when interacted with, it would display the 3 options: Draft, Pin, Dismiss
This would allow us to keep the interface clean on all viewports but also could be a pattern we use throughout the plugin.

If possible, we could use an animation to show what's happening with the element, i.e. Dismiss would move the element to the left out of the list, Pin would move it to the right (as going into the other tab) and Draft would magnify it to take the users to the other screen.

@marrrmarrr
Copy link
Collaborator

+1 to Alex's comment, it will save a lot of space in the mobile UI.

@johnPhillips
Copy link
Contributor

Thanks @alex-moulin!

Two nice ideas: I will give my thoughts on each but ultimately I'll defer to Felix on how he wants to proceed.

show 3 vertical dots on hover, and when interacted with, it would display the 3 options: Draft, Pin, Dismiss

I think this is a really nice idea. Do you mean like the button with 3 dots used in Gmail?

Icon button Menu component
Screenshot 2021-08-26 at 15 00 35 Screenshot 2021-08-26 at 15 08 41

If so, we already use Material Design components for the UI so could use Icon Button for the 3 dots and then the Menu component to show the actions, for example.

Is that what you had in mind? Very happy to knock together a demo if required.

If possible, we could use an animation to show what's happening with the element, i.e. Dismiss would move the element to the left out of the list, Pin would move it to the right (as going into the other tab) and Draft would magnify it to take the users to the other screen.

I agree that some sort of visual feedback would be great. Currently the idea just disappears, and apart from a counter incrementing on a neighbouring tab there isn't really any sense what happened. We could try adding some transitions and see how it looks. Another idea might be to use Material's Snackbar component to provide feedback.

Over to you @felixarntz - how would you like me to proceed?

@alex-moulin
Copy link

@johnPhillips just to confirm this is indeed what I had in mind regarding the 3 dots and Menu component :)

@felixarntz
Copy link
Member

Per #3839 (comment), we may actually want to circle back and go with the original PR approach here.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

This actually looks good to me while testing! Paging @marrrmarrr for an additional approval.

@marrrmarrr
Copy link
Collaborator

Just tested it too, looks good!

@felixarntz felixarntz merged commit 981c30d into develop Sep 1, 2021
@felixarntz felixarntz deleted the fix/3839-overlapping-icons branch September 1, 2021 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants