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

Add theme support to tooltip component #5326

Merged
merged 4 commits into from
Sep 5, 2024

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Aug 26, 2024

Done

  • Updates tooltips to invert their ancestor's color theme. I.E.: tooltips on dark use light theme, tooltips on light/paper use dark theme.
  • Tooltips can be set to use a different theme than the inverse of the body, to handle cases where the tooltip is contained within an element that has a theme different from the body.

Fixes WD-11871

QA

  • Verify that all tooltip examples look correct on all color themes. They should always use the opposite color theme.

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix release (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshots

image
Screenshot 2024-08-30 at 10 19 36 AM

@webteam-app
Copy link

@jmuzina jmuzina marked this pull request as ready for review August 27, 2024 18:46
Copy link
Contributor

@pastelcyborg pastelcyborg left a comment

Choose a reason for hiding this comment

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

Nit: The title of this PR is a bit odd - "Add theme support to tooltip component" is probably fine.

Also, it's a bit strange that the tooltip body is visually disconnected from its tail:

image

Not directly related to this PR, I don't think, but fixing this might be a nice drive-by

@jmuzina jmuzina changed the title Cleanup color vars in tooltip styles Add theme support to tooltip component Aug 27, 2024
@bartaz
Copy link
Member

bartaz commented Aug 30, 2024

The contrast between dark tooltip and dark background doesn't seem sufficient. Tooltip is barely visible.

image

@lyubomir-popov Any thoughts? Not sure how often tooltips are used on top of dark backgrounds yet (so we may not need to address this right away), but this doesn't seem to be a good long term solution.

Or maybe the issue is the contrast ratio between dark and alternative dark backgrounds? But obviously changing those will affect many other places.

@lyubomir-popov
Copy link
Contributor

Or maybe the issue is the contrast ratio between dark and alternative dark backgrounds? But obviously changing those will affect many other places.

let's just flip to the opposite theme in tooltips, iirc lots of people felt that to be the right choice last time we looked at these.

@jmuzina
Copy link
Member Author

jmuzina commented Aug 30, 2024

Or maybe the issue is the contrast ratio between dark and alternative dark backgrounds? But obviously changing those will affect many other places.

let's just flip to the opposite theme in tooltips, iirc lots of people felt that to be the right choice last time we looked at these.

@lyubomir-popov This has been updated to invert the tooltip color themes.

pastelcyborg
pastelcyborg previously approved these changes Sep 2, 2024
Copy link
Contributor

@pastelcyborg pastelcyborg left a comment

Choose a reason for hiding this comment

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

Overall, the theming functionality looks good now, but that disconnected tail seems like a thing we should fix here, if possible.

@jmuzina
Copy link
Member Author

jmuzina commented Sep 3, 2024

Overall, the theming functionality looks good now, but that disconnected tail seems like a thing we should fix here, if possible.

@pastelcyborg
@bartaz and I discussed this briefly last week and I forgot to note it here: this only seems to happen when you zoom the page beyond 100%. Does it occur on other magnifications for you? If so, on what browser(s)?

@bartaz
Copy link
Member

bartaz commented Sep 4, 2024

Detached tooltip is not white?

image

@jmuzina
Copy link
Member Author

jmuzina commented Sep 4, 2024

Detached tooltip is not white?

@bartaz

I think they are the same #f7f7f7 - maybe the different background contrast makes them look different?

Screenshot 2024-09-04 at 9 25 20 AM Screenshot 2024-09-04 at 9 25 30 AM

@lyubomir-popov can you verify whether these tooltips look correct?

@jmuzina
Copy link
Member Author

jmuzina commented Sep 4, 2024

@bartaz @pastelcyborg Updated this to support & document theme inversion override as discussed today:
Screenshot 2024-09-04 at 2 41 04 PM

@jmuzina
Copy link
Member Author

jmuzina commented Sep 4, 2024

@pastelcyborg @bartaz Do you think this is more clear semantically if the .p-tooltip with overridden background uses the .is-<theme> of the background that is actually displayed for the tooltip? I wonder if it's a bit confusing that .p-tooltip.is-light results in a tooltip with a dark background

@pastelcyborg
Copy link
Contributor

@pastelcyborg @bartaz Do you think this is more clear semantically if the .p-tooltip with overridden background uses the .is-<theme> of the background that is actually displayed for the tooltip? I wonder if it's a bit confusing that .p-tooltip.is-light results in a tooltip with a dark background

I would stick with the solution we've come up with. The theme values represent the component when used with a particular theme, not the current color scheme most visually prevalent within the component.

@lyubomir-popov
Copy link
Contributor

@jmuzina looks good, only spotted one thing - the incorrect example at the bottom works the same way as the correct one:
image

https://vanilla-framework-5326.demos.haus/docs/examples/patterns/tooltips/combined?theme=light

@jmuzina
Copy link
Member Author

jmuzina commented Sep 5, 2024

@jmuzina looks good, only spotted one thing - the incorrect example at the bottom works the same way as the correct one: image

https://vanilla-framework-5326.demos.haus/docs/examples/patterns/tooltips/combined?theme=light

@lyubomir-popov It depends on the theme of the document body, try switching the page theme and you'll see it changes.

The one with the dark strip only changes between the two tooltips when the page itself is light-themed

Copy link
Contributor

@lyubomir-popov lyubomir-popov left a comment

Choose a reason for hiding this comment

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

LGTM

@pastelcyborg pastelcyborg added Review: QA +1 Review: Percy +1 and removed Review: QA needed Review: Percy needed This PR needs a review of Percy for visual regressions labels Sep 5, 2024
@jmuzina jmuzina merged commit 2d015f7 into canonical:main Sep 5, 2024
11 checks passed
@jmuzina jmuzina deleted the theme-tooltips branch September 5, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants