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: Ensure tooltip doesn't override its anchor's ref #665

Merged
merged 4 commits into from
May 27, 2022

Conversation

frankieyan
Copy link
Member

@frankieyan frankieyan commented May 26, 2022

Short description

In #662 I made a mistake in the Tooltip component, which causes it to override the ref being passed to its anchor element. This breaks components that both require a tooltip, and another component that needs it as an anchor. For example, the scheduler in the new task detail's sidebar, which uses popper to display the scheduler menu. By losing the ref, the scheduler menu can't be positioned correctly near its trigger.

Unfortunately I couldn't test the build in-app as I'm running into issues with react-focus-lock once I pull in this branch through npm link (ide run --link-reactist), as Todoist is running a newer version and is able to dedupe the different copies when installing through npm normally. Even when updating the version here to match, I'm seeing weird behaviour once linked into the app. I'll have to test again once this is released.

Test plan

Menu

  • With the "With sub-menu" example, a tooltip appears when you focus on the menu trigger
    • Pressing enter to select a menu item should place the focus back on the trigger, but not show the tooltip

Tooltip

  • Hovering or focusing on the box shows its tooltip

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Updated all static build artifacts (npm run build-all)

Versioning

Patch

@frankieyan frankieyan force-pushed the frankie/fix-tooltip-ref branch from 8095815 to 2b373f2 Compare May 26, 2022 18:22
@frankieyan frankieyan requested review from a team and pauloslund and removed request for a team May 26, 2022 18:25
@frankieyan frankieyan self-assigned this May 26, 2022
@frankieyan
Copy link
Member Author

@pauloslund I'm going to release this as a patch as it's blocking some of my work. Let me know if you have anything you'd like to see addressed when you get a chance 👍

@frankieyan frankieyan merged commit ede8261 into main May 27, 2022
@frankieyan frankieyan deleted the frankie/fix-tooltip-ref branch May 27, 2022 12:10
Copy link
Contributor

@pauloslund pauloslund left a comment

Choose a reason for hiding this comment

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

👍

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.

2 participants