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.

Sign up for GitHub

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

Jump to bottom

fix(FloatingMenu): lazy evaluate trigger button position #5881

Merged
merged 2 commits into from
Apr 17, 2020
Merged

fix(FloatingMenu): lazy evaluate trigger button position #5881

merged 2 commits into from
Apr 17, 2020

Conversation

asudoh
Copy link
Contributor

@asudoh asudoh commented Apr 17, 2020

This change switches the position calculation logic for the trigger buttons of <OverflowMenu> and <Tooltip> from eager-evaluation on mount to lazy-evaluation on rendering <FloatingMenu>.

This change relegates such logic from <OverflowMenu> and <Tooltip> to <FloatingMenu> to do that.

Such change has two merits:

  1. Avoids stale trigger position data, which happened when <Tooltip> is opened programmatically
  2. Reduces getBoundingClientRect() calls when application is initialized

Also, swiches <FloatingMenu>'s prop for the DOM element ref of trigger button, as well as the corresponding internal properties of <OverflowMenu>/<Tooltip> from raw element to React ref.

Fixes #5305.

Changelog

Changed

  • Lazy-evaluate the position of trigger button for floating menus.
  • Switch from raw DOM element refs to React refs in several floating menu code.

Testing / Reviewing

Testing should make sure <OverflowMenu> and <Tooltip> are not broken.

@asudoh
fix(FloatingMenu): lazy evaluate trigger button position
3fa9408 
This change switches the position calculation logic for the trigger
buttons of `<OverflowMenu>` and `<Tooltip>` from eager-evaluation on
mount to lazy-evaluation on rendering `<FloatingMenu>`.

This change relegates such logic from `<OverflowMenu>` and `<Tooltip>`
to `<FloatingMenu>` to do that.

Such change has two merits:

1. Avoids stale trigger position data, which happened when `<Tooltip>`
   is opened programmatically
2. Reduces `getBoundingClientRect()` calls when application is
   initialized

Also, swiches `<FloatingMenu>`'s prop for the DOM element ref of
trigger button, as well as the corresponding internal properties of
`<OverflowMenu>`/`<Tooltip>` from raw element to React ref.

Fixes carbon-design-system#5305.
@asudoh asudoh requested review from emyarod and a team April 17, 2020 07:36
@ghost ghost requested review from joshblack and removed request for a team April 17, 2020 07:36
@netlify
Copy link

netlify bot commented Apr 17, 2020 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit b221a2b

https://deploy-preview-5881--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 17, 2020 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit b221a2b

https://deploy-preview-5881--carbon-components-react.netlify.app

emyarod
emyarod approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me

@asudoh asudoh requested a review from a team as a code owner April 17, 2020 22:59
@ghost ghost requested review from aledavila and tw15egan April 17, 2020 22:59
@asudoh asudoh mentioned this pull request Apr 17, 2020
Closed
@asudoh asudoh merged commit fb3b4b5 into carbon-design-system:master Apr 17, 2020
@asudoh asudoh deleted the floating-menu-lazy-evaluate-trigger-position branch April 17, 2020 23:44
joshblack pushed a commit to joshblack/carbon that referenced this pull request Apr 23, 2020
@asudoh @joshblack
fix(FloatingMenu): lazy evaluate trigger button position (carbon-desi…
42afc61 
…gn-system#5881)

This change switches the position calculation logic for the trigger
buttons of `<OverflowMenu>` and `<Tooltip>` from eager-evaluation on
mount to lazy-evaluation on rendering `<FloatingMenu>`.

This change relegates such logic from `<OverflowMenu>` and `<Tooltip>`
to `<FloatingMenu>` to do that.

Such change has two merits:

1. Avoids stale trigger position data, which happened when `<Tooltip>`
   is opened programmatically
2. Reduces `getBoundingClientRect()` calls when application is
   initialized

Also, swiches `<FloatingMenu>`'s prop for the DOM element ref of
trigger button, as well as the corresponding internal properties of
`<OverflowMenu>`/`<Tooltip>` from raw element to React ref.

Fixes carbon-design-system#5305.
@joshblack joshblack mentioned this pull request Apr 23, 2020
Closed
joshblack pushed a commit to joshblack/carbon that referenced this pull request Apr 23, 2020
@asudoh @joshblack
fix(FloatingMenu): lazy evaluate trigger button position (carbon-desi…
e67a2e9 
…gn-system#5881)

This change switches the position calculation logic for the trigger
buttons of `<OverflowMenu>` and `<Tooltip>` from eager-evaluation on
mount to lazy-evaluation on rendering `<FloatingMenu>`.

This change relegates such logic from `<OverflowMenu>` and `<Tooltip>`
to `<FloatingMenu>` to do that.

Such change has two merits:

1. Avoids stale trigger position data, which happened when `<Tooltip>`
   is opened programmatically
2. Reduces `getBoundingClientRect()` calls when application is
   initialized

Also, swiches `<FloatingMenu>`'s prop for the DOM element ref of
trigger button, as well as the corresponding internal properties of
`<OverflowMenu>`/`<Tooltip>` from raw element to React ref.

Fixes carbon-design-system#5305.
@joshblack joshblack mentioned this pull request Apr 23, 2020
Merged
joshblack pushed a commit that referenced this pull request Apr 23, 2020
@asudoh @joshblack
fix(FloatingMenu): lazy evaluate trigger button position (#5881)
a4967b2 
This change switches the position calculation logic for the trigger
buttons of `<OverflowMenu>` and `<Tooltip>` from eager-evaluation on
mount to lazy-evaluation on rendering `<FloatingMenu>`.

This change relegates such logic from `<OverflowMenu>` and `<Tooltip>`
to `<FloatingMenu>` to do that.

Such change has two merits:

1. Avoids stale trigger position data, which happened when `<Tooltip>`
   is opened programmatically
2. Reduces `getBoundingClientRect()` calls when application is
   initialized

Also, swiches `<FloatingMenu>`'s prop for the DOM element ref of
trigger button, as well as the corresponding internal properties of
`<OverflowMenu>`/`<Tooltip>` from raw element to React ref.

Fixes #5305.
@emyarod emyarod mentioned this pull request Sep 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshblack joshblack joshblack approved these changes

@emyarod emyarod emyarod approved these changes

@aledavila aledavila Awaiting requested review from aledavila

@tw15egan tw15egan Awaiting requested review from tw15egan

Assignees
No one assigned
Labels
status: ready for review 👀
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tooltip positioning problem, when starting offscreen
3 participants
@asudoh @joshblack @emyarod