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

fix: tooltip position related styles not applied #4181

Merged
merged 2 commits into from
Dec 15, 2020

Conversation

scomea
Copy link
Collaborator

@scomea scomea commented Dec 11, 2020

Description

Tooltip css selectors were based on the user settings on attributes so weren't being applied when the attributes were absent. Switched to use anchored-region classes instead.

Motivation & context

noticed a bug

Issue type checklist

  • Chore: A change that does not impact distributed packages.
  • Bug fix: A change that fixes an issue, link to the issue above.
  • New feature: A change that adds functionality.

Is this a breaking change?

  • This change causes current functionality to break.

Adding or modifying component(s) in @microsoft/fast-components checklist

Process & policy checklist

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

@nicholasrice
Copy link
Contributor

This change reverts the work done in #4078, which I don't think is what we want. The goal of that PR was to move away from setting the class attribute on the host because clobbering of those classes happens easily when used with some UI frameworks.

I think instead that the relevant properties should always be reflected to attributes, ensuring that the attrs can always be used for styling.

@scomea
Copy link
Collaborator Author

scomea commented Dec 14, 2020

@nicholasrice In this case we want to set the styles based on the actual position of the tooltip which is most reliably retrieved by selecting based on the internal anchored region's classes. The tooltip's "position" attribute doesn't work for this as currently implemented because if it is "undefined" the toolip dynamically places itself above/below the anchor ("position" remains undefined). There currently is not an attribute on tooltip that advertises the actual position in the dynamic case, probably should be, but even if we add that I'd still consider the internal anchored region the most stable indicator of how to style margins based on placement.

Copy link
Contributor

@nicholasrice nicholasrice left a comment

Choose a reason for hiding this comment

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

Yep, you're right. I mistook this for requiring hoisting classes to the tooltip but that's not what is going on. Looks good.

@scomea scomea merged commit 0370b08 into master Dec 15, 2020
@scomea scomea deleted the users/scomea/tooltip-margin-fix branch December 15, 2020 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants