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

Tweak margin and padding of toolbar icon buttons #18144

Closed

Conversation

enriquesanchez
Copy link
Contributor

@enriquesanchez enriquesanchez commented Oct 28, 2019

Fixes 1/3 of #12260

Description

This PR tweaks the margin and padding of the toolbar icon buttons in order to give their corresponding tooltips more breathing room when hovering.

How has this been tested?

Tested on Firefox, Safari and Chrome (Mac), on large and narrow viewports.

Screenshots

Before:

Screen Shot 2019-10-28 at 1 18 46 PM

Screen Shot 2019-10-28 at 1 18 53 PM

After:

Screen Shot 2019-10-25 at 4 36 13 PM

Screen Shot 2019-10-28 at 12 49 40 PM

Screen Shot 2019-10-28 at 1 17 06 PM

Screen Shot 2019-10-28 at 1 17 25 PM

Screen Shot 2019-10-28 at 1 17 16 PM

Screen Shot 2019-10-28 at 1 16 58 PM

Types of changes

CSS and visual.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@JJJ
Copy link
Contributor

JJJ commented Oct 28, 2019

Hovering over Button:
Screen Shot 2019-10-28 at 3 05 45 PM

Hovering over Tooltip:
Screen Shot 2019-10-28 at 3 05 55 PM


If the tooltip is purposely remaining hoverable, and if clicking the tooltip is purposely invoking a click on the thing it is a tooltip for, then this PR provides a nice visual improvement, but does not address the interaction bugs. This looks much better though!

@gziolo gziolo added [Feature] UI Components Impacts or related to the UI component system [Type] Bug An existing feature does not function as intended labels Oct 30, 2019
@mapk
Copy link
Contributor

mapk commented Oct 31, 2019

I agree this looks better, however it doesn't yet solve #12260. @enriquesanchez can we also explore keeping the hover state when hovering a tooltip in this PR?

@enriquesanchez
Copy link
Contributor Author

I'm having trouble figuring out how to get the button to stay hovered while also hovering the tooltip.

@ItsJonQ do you think you could help me? We want to have the button maintain its hover state while the tooltip is being hovered.

@ItsJonQ
Copy link

ItsJonQ commented Nov 13, 2019

@enriquesanchez Haii!! Without hacking the JS directly... For CSS work, the best work-around I found for these interactions is...

  • Inspect Element via Chrome
  • Click on Sources
  • Hover UI to trigger Tooltip
  • Press F8 on your keyboard

This freezes the document, keeping the tooltip open.
You can then inspect element to adjust CSS as needed

Screen Shot 2019-11-13 at 12 27 43 PM

Hope this helps 🙏

@mapk
Copy link
Contributor

mapk commented Nov 22, 2019

@ItsJonQ f8 didn't work for me. I have to use setTimeout(() => { debugger; }, 5000) and then hover over the element.

I think the problem with this is because the tooltip HTML doesn't appear inside the icon's HTML. So as soon as you hover away from the icon, it no longer shows as hovered. It's not behaving like a dropdown menu might. Is there a way to move the HTML inside the icon so it acts like a dropdown?

@enriquesanchez
Copy link
Contributor Author

@ItsJonQ @mapk Yep, sorry I wasn't clear enough. Mark's comment is what I meant. Because the button and the tooltip don't have a parent-child relationship, hovering the tooltip removes hover from the button. Ideally, we can keep both hovered instead.

@ItsJonQ
Copy link

ItsJonQ commented Dec 2, 2019

Is there a way to move the HTML inside the icon so it acts like a dropdown?

@mapk We actually don't want that to happen. As far as App UI goes, it's preferable for the dropdown/popover box to be rendered at the root level. The reason is to better consistency + control over styles. Otherwise, nested CSS or things like position or overflow:hidden would break the dropdown/popover.

Ideally, we can keep both hovered instead.

@enriquesanchez To do this, we'll need to update the JS code for the Tooltip/Popover system to allow for this :)

@ZebulanStanphill
Copy link
Member

Now that the G2 UI changes have landed in master, is this PR still relevant?

@mapk
Copy link
Contributor

mapk commented Mar 6, 2020

This PR doesn't look relevant anymore.

G2 tooltips

Screen Shot 2020-03-06 at 12 26 40 PM

Screen Shot 2020-03-06 at 12 26 52 PM

@mapk mapk closed this Mar 6, 2020
@aristath aristath deleted the fix/slightly-move-down-top-toolbar-tooltips branch November 10, 2020 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants