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): custom animation distance overrides overlay value #2266

Merged
merged 4 commits into from
Nov 9, 2023

Conversation

mdt2
Copy link
Collaborator

@mdt2 mdt2 commented Nov 6, 2023

Description

Per issue #2156, Tooltips (all variants except the hover variant) were inheriting the wrong animation distance (6px instead of the 4px that is noted in the spec). This work adjusts the animation distance for the component.

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

  • Use dev tools to inspect a tooltip. You should see the animation distance transform at the top level of the component being set with the --spectrum-tooltip-animation-distance token.

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

components/tooltip/index.css Outdated Show resolved Hide resolved
@mdt2 mdt2 marked this pull request as ready for review November 6, 2023 19:43
Copy link
Contributor

github-actions bot commented Nov 6, 2023

🚀 Deployed on https://pr-2266--spectrum-css.netlify.app

@github-actions github-actions bot temporarily deployed to pull request November 6, 2023 19:44 Inactive
@mdt2 mdt2 requested a review from jawinn November 7, 2023 19:25
@github-actions github-actions bot temporarily deployed to pull request November 7, 2023 19:25 Inactive
@mdt2 mdt2 mentioned this pull request Nov 7, 2023
23 tasks
Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

It would be useful to compare the actual output here for review, to ensure we're not seeing any values forced on us from the mixins leveraged here?

Is there benefit from using --spectrum-overlay-animation-distance or a similar value in the various directions rather than all of var(--mod-tooltip-animation-distance, var(--spectrum-tooltip-animation-distance)) each time?

@mdt2
Copy link
Collaborator Author

mdt2 commented Nov 7, 2023

@Westbrook hello! Yes, below is the dist output on prod vs this PR. Before this PR, we were relying on some transform values in "../commons/overlay-coretokens.css", which is what caused the wonkiness you were seeing in the issue you posted. You can see this in the prod output that it's falling back to the 6px instead of the spacing-75 4px animation distance value that we set for Tooltip.

Instead of trying to force the animation-distance to be something different than in overlay-coretokens.css, I've refactored the Tooltip to define it's own transforms so that we're no longer inheriting those. This eliminates the connection to --spectrum-overlay-animation-distance and hopefully also eliminates the confusion caused by the inheritance.

Let me know what other questions you have.

tooltip-dist

@github-actions github-actions bot temporarily deployed to pull request November 7, 2023 20:54 Inactive
@@ -122,6 +119,7 @@ governing permissions and limitations under the License.
}

&.is-open {
--spectrum-overlay-animation-distance: var(--mod-tooltip-animation-distance, var(--spectrum-tooltip-animation-distance));
Copy link
Contributor

Choose a reason for hiding this comment

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

Overwriting this value in the .is-open state means that JS application will not have access to this value while the Tooltip is closed. I can't remember right off why someone was asking about that recently, but would it be that different to move this onto the default rule?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I either didn't send this earlier or it disappeared, so for posterity:

Good catch! We don't need this overwrite since we've refactored the transforms so I removed it.

@mdt2 mdt2 force-pushed the mdt2/css-635-tooltip-animation-distance branch from 08e2112 to b9aa8b1 Compare November 7, 2023 22:05
@github-actions github-actions bot temporarily deployed to pull request November 7, 2023 22:09 Inactive
Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

LGTM!

@pfulton pfulton force-pushed the mdt2/css-635-tooltip-animation-distance branch from b9aa8b1 to e8ac3ad Compare November 9, 2023 20:03
@github-actions github-actions bot temporarily deployed to pull request November 9, 2023 20:08 Inactive
@pfulton pfulton force-pushed the mdt2/css-635-tooltip-animation-distance branch from e8ac3ad to 8916055 Compare November 9, 2023 20:32
@github-actions github-actions bot temporarily deployed to pull request November 9, 2023 20:36 Inactive
@pfulton pfulton added the run_vrt For use on PRs looking to kick off VRT label Nov 9, 2023
@github-actions github-actions bot removed the run_vrt For use on PRs looking to kick off VRT label Nov 9, 2023
@pfulton pfulton merged commit 8916055 into main Nov 9, 2023
7 checks passed
@pfulton pfulton deleted the mdt2/css-635-tooltip-animation-distance branch November 9, 2023 20:50
@github-actions github-actions bot temporarily deployed to production November 9, 2023 20:55 Inactive
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.

None yet

4 participants