-
Notifications
You must be signed in to change notification settings - Fork 166
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contrast between dark tooltip and dark background doesn't seem sufficient. Tooltip is barely visible. @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. |
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. |
2042de8
to
8dfe619
Compare
There was a problem hiding this 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.
@pastelcyborg |
I think they are the same @lyubomir-popov can you verify whether these tooltips look correct? |
8dfe619
to
0e7c356
Compare
0e7c356
to
4266879
Compare
@bartaz @pastelcyborg Updated this to support & document theme inversion override as discussed today: |
@pastelcyborg @bartaz Do you think this is more clear semantically if the |
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. |
@jmuzina looks good, only spotted one thing - the incorrect example at the bottom works the same way as the correct one: 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Done
Fixes WD-11871
QA
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:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver convention:Screenshots