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

Icons overlapping idea title. #3839

Closed
asvinb opened this issue Aug 12, 2021 · 14 comments
Closed

Icons overlapping idea title. #3839

asvinb opened this issue Aug 12, 2021 · 14 comments
Labels
Module: Idea Hub Google Idea Hub module related issues P1 Medium priority Rollover Issues which role over to the next sprint Type: Bug Something isn't working
Milestone

Comments

@asvinb
Copy link
Collaborator

asvinb commented Aug 12, 2021

Bug Description

When the browser is resized to 960px, upon hovering an idea, the icons are displayed on top of the idea name.

Steps to reproduce

  1. Go to Site Kit dashboard
  2. Scroll down to Idea Hub widget
  3. Resize browser to 960px
  4. Hover a suggested idea.
  5. See error

Screenshots

Additional Context

  • Browser: Chrome

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

Acceptance criteria

  • For viewports where the Idea Hub widget appears in a narrow state (even if this viewport is technically not "mobile"), the buttons on each idea should appear below the idea rather than on the right of the idea, similar to how it is in the current mobile layout.

Implementation Brief

Using assets/js/modules/idea-hub/components/dashboard/DashboardIdeasWidget/Idea.js and assets/sass/components/idea-hub/_googlesitekit-idea-hub-dashboard-ideas-widget.scss:

  • Replace the grid layout with normal divs:
    • Replace the <Grid> and <Cell> with <div>s with the same className attributes (.googlesitekit-idea-hub__idea--actions and .googlesitekit-idea-hub__idea--details respectively).
    • The <Row> can be replaced with a div with a suitable className (div.googlesitekit-idea-hub__idea--wrapper for example).
    • Remove any mdc-layout-grid-* CSS that is now redundant.
  • Refactor the layout CSS:
    • For viewports below $bp-xlarge (1280), use a display: block-based layout, such that the .googlesitekit-idea-hub__idea--actions wraps underneath the .googlesitekit-idea-hub__idea--details.
    • For viewports above $bp-xlarge, switch to a flex-based layout, such that the two divs are inline
    • The media query which adds a display: none rule to the .googlesitekit-idea-hub__actions--delete, .googlesitekit-idea-hub__actions--pin and .googlesitekit-idea-hub__actions--unpin should be updated so that it applies to viewports above $bp-xlarge. This means that the action buttons are all visible when they wrap.
    • Similarly, the media query that applies justify-content: flex-end; to .googlesitekit-idea-hub__actions should be updated to apply to viewports above $bp-xlarge, so that the action buttons are left-aligned when they wrap.

Here is a gist:

	// Replace grid layout with normal divs
	return (
		<div className="googlesitekit-idea-hub__idea--single">
			<div className="example-css">
				<div className="googlesitekit-idea-hub__idea--details"> ... </div>
				<div className="googlesitekit-idea-hub__idea--actions"> ... </p>
			</div>
		</div>
@media (min-width: $bp-xlarge) {
	/* Use a flex layout above 1200 */
	.example-css {
		display: flex;
		padding: 12px 24px; /* use variables, this is just to illustrate */
		justify-content: space-between;
	}

	/* The following action buttons are hidden above 1200 */
	.googlesitekit-idea-hub__actions--delete,
	.googlesitekit-idea-hub__actions--pin,
	.googlesitekit-idea-hub__actions--unpin {
			display: none;
	}

	/* The  actions are right-aligned */
	.googlesitekit-idea-hub__idea--actions {
		justify-content: flex-end;
	}
}

And an example screenshot:

Below 1200 Above 1200
Screenshot 2021-08-18 at 16 09 40 Screenshot 2021-08-18 at 16 15 23

Test Coverage

  • No new tests required.

Visual Regression Changes

  • Images dealing with the Idea Hub Dashboard widget might need to be updated.

QA Brief

  • Check that the icons in the Idea Hub dashboard widget does not overlap the text content across all breakpoints.

Changelog entry

  • Ensure the icon buttons in the Idea Hub widget do not overlap idea labels.
@felixarntz felixarntz self-assigned this Aug 12, 2021
@felixarntz felixarntz added Module: Idea Hub Google Idea Hub module related issues P1 Medium priority Type: Bug Something isn't working labels Aug 12, 2021
@fhollis fhollis added this to the Sprint 56 milestone Aug 12, 2021
@felixarntz felixarntz removed their assignment Aug 12, 2021
@johnPhillips
Copy link
Contributor

johnPhillips commented Aug 13, 2021

For viewports where the Idea Hub widget appears in a narrow state (even if this viewport is technically not "mobile"), the buttons on each idea should appear the same way as in the current mobile layout, below the idea rather than on the right of the idea.

@felixarntz The current layout uses MDC grid, which is based on media queries (as far as I can make out). In a sense, media queries are a bit redundant, because the widget can be 'mobile width' on a desktop viewport under the circumstances described (if alongside another widget).

So we need a way to activate the mobile layout based on the width of the widget rather than the viewport. The only way I can think of to do this is to scrap the MDC grid and get the content to overflow and wrap as the container shrinks? So the layout issue can be solved this way, with the buttons tucking under the idea content as the width decreases.

However, we have additional styles that kick in on mobile media queries that would be impossible to replicate in this way, for example the buttons are all visible on smaller viewports, whereas on desktop they are hidden unless you hover over them. I can't think of a way to have buttons hidden when they are inline but visible when they overflow. If only @container queries were available.

TD;DR I'm pretty sure we would need to make the buttons visible across all device widths to make this work. That's a pretty significant change to the design though, so I wanted to flag it to you before writing the IB.

In the example below I've tried to demo this, just for clarity.

  • The first item is how it might look inside a wider widget, with everything on one line
  • The second item is what it might look like when the widget gets narrower and wraps
  • Note that the buttons are visible throughout because without media queries we can't have the buttons hidden on desktop and visible on mobile; they need to be visible all the time.

Screenshot 2021-08-13 at 16 14 27

Worth checking with @asvinb too as his CSS skills are mightier than mine. Am I right in saying this?

@johnPhillips johnPhillips self-assigned this Aug 13, 2021
@felixarntz
Copy link
Member

@johnPhillips Thanks for the detailed insights - I may have phrased the ACs overly broadly, the main thing we should accomplish is indeed for the buttons to appear in their own line below the idea for these viewport sizes. They don't need to 100% match the exact styles used on mobile, the goal is to prevent the overlap.

@johnPhillips
Copy link
Contributor

Just to explain the consensus I arrived at with @asvinb, @aaemnnosttv and @eugene-manuilov in a slack conversation:

  • We should wrap the action buttons at a suitable breakpoint (Asvin suggested 1200)
  • The action buttons should only be hidden when inline, as they look a bit strange if they are hidden when wrapped (see second idea in screenshot below):

Screenshot 2021-08-18 at 14 32 40

I have updated the IB accordingly.

@tofumatt tofumatt self-assigned this Aug 18, 2021
@tofumatt
Copy link
Collaborator

This works for me—thanks for the detailed discussion and reasoning behind the approach in the comments, it helped a lot! 👍🏻

IB ✅

@felixarntz
Copy link
Member

@marrrmarrr @alex-moulin I pinged you on the PR for this #3920, can you share your thoughts there?

@eclarke1
Copy link
Collaborator

@felixarntz for this one, both @marrrmarrr and @alex-moulin have responded on #3920
@johnPhillips could this one potentially come back to you for Execution as Asvin is now away please? (To implement the suggestion from Alex?)

@eclarke1 eclarke1 assigned johnPhillips and unassigned marrrmarrr Aug 26, 2021
@aaemnnosttv
Copy link
Collaborator

@eclarke1 I don't think we can implement the suggested changes without more definition. Ideally we should establish some secondary ACs and design to work from in which case it might make more sense to address in a follow up issue. Cc: @felixarntz

@johnPhillips johnPhillips removed their assignment Aug 26, 2021
@eclarke1 eclarke1 added the Rollover Issues which role over to the next sprint label Aug 31, 2021
@johnPhillips
Copy link
Contributor

johnPhillips commented Aug 31, 2021

I've created a little demo of @alex-moulin's idea hidden menu idea here.

https://google.github.io/site-kit-wp/storybook/pull/3966/?path=/story/modules-idea-hub-widgets-dashboardideaswidget--ready

Smaller screen Larger screen
Screenshot 2021-08-31 at 13 32 58 Screenshot 2021-08-31 at 13 33 18

Notes:

@eclarke1 eclarke1 modified the milestones: Sprint 56, Sprint 57 Aug 31, 2021
@felixarntz
Copy link
Member

@johnPhillips Thanks, this works for me. Paging @marrrmarrr @alex-moulin to share their thoughts. It's somewhat urgent to make a decision here, is the above good to proceed? It's a rather drastic UI change of course, so I want to make sure we're all aligned.

@felixarntz felixarntz assigned marrrmarrr and unassigned felixarntz Aug 31, 2021
@alex-moulin
Copy link

@johnPhillips @felixarntz this mock-up looks good to me.
If I could be picky then I would suggest we add a label next to each action on this menu as we got the space so users know exactly what's available to them.
cc. @marrrmarrr

@felixarntz
Copy link
Member

felixarntz commented Sep 1, 2021

@johnPhillips @alex-moulin After reviewing again with @marrrmarrr, we've decided to go back to the original scope of the issue here and not change the design significantly at this point - the small menu also has some issues, e.g. it requires extra clicks to access, and a menu of vertical icons feels odd, e.g. since there is now an additional icon among them to just close the menu.

At this point, something like the solution from #3839 (comment) is what we should go with.

In other words, let's now follow the original ACs on the issue and create a PR for that. @johnPhillips It's okay to remove the hover state for now so that the icons always show for both mobile and desktop. When the PR is testable / previewable (e.g. on Storybook), we can make a final decision (i.e. make sure to request review by @marrrmarrr and myself), but it seems to me that removing the hover effect is the appropriate solution here, also since it will unblock #3907.

@felixarntz
Copy link
Member

@johnPhillips Actually 🤦 I forgot there already was a PR for this one. #3920 actually looks good to me as is when focused on purely the issue here. IMO we can move forward with that, unless there were outstanding problems? I know we still have the challenge of the hover state complicating #3907, but if it's for that, then we should discuss that part further in #3907 and keep it out of this issue here.

@felixarntz
Copy link
Member

That escalated quickly! PR is merged 😆

@wpdarren wpdarren self-assigned this Sep 2, 2021
@wpdarren
Copy link
Collaborator

wpdarren commented Sep 2, 2021

QA Update: ✅

Verified that the icons in the Idea Hub dashboard widget does not overlap the text content across all breakpoints.

ih-1.mp4

@wpdarren wpdarren removed their assignment Sep 2, 2021
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 P1 Medium priority Rollover Issues which role over to the next sprint Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests