Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(Popup): avoid double rendering #1153

Merged
merged 15 commits into from
May 9, 2019
Merged

fix(Popup): avoid double rendering #1153

merged 15 commits into from
May 9, 2019

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Apr 2, 2019

BREAKING CHANGES

defaultTarget prop is removed on Popup component, please use directly target prop.


Goal

This PR backports changes from SUIR. It allows us to avoid double rendering of Popup because we don't store target in state anymore. target is no longer autocontrolled prop because we don't need in component's state.

It uses an idea with VirtualReference from react-popper, so we can can use createRef() API from React and avoid double rendering as we don't need to store target in components state more.


Additional: fix Dropdown items' positioning

Also, these changes are needed for #1312 : to render dropdown items in the correct area based on align and placement props and Popper integration

What

  • currently everything seems to be working, but this is because of double render, as we usually open the Dropdown by mouse/keyboard (causing a double render);
  • however, if we set open flag to be always true on a Dropdown, then PopperJS does not render the items on first render

Note on alternative solution

The alternative would be to introduce logic to render the Dropdown items only when the Dropdown open flag is true; but this breaks accessibility concerns, as the list of Dropdown items always needs to be open

@codecov
Copy link

codecov bot commented Apr 2, 2019

Codecov Report

Merging #1153 into master will increase coverage by 0.53%.
The diff coverage is 64.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1153      +/-   ##
==========================================
+ Coverage   72.24%   72.78%   +0.53%     
==========================================
  Files         752      748       -4     
  Lines        5614     5665      +51     
  Branches     1671     1633      -38     
==========================================
+ Hits         4056     4123      +67     
+ Misses       1551     1535      -16     
  Partials        7        7
Impacted Files Coverage Δ
packages/react/src/components/Popup/Popup.tsx 68.29% <100%> (-0.76%) ⬇️
...src/components/Popup/createPopperReferenceProxy.ts 45.45% <45.45%> (ø)
...t/src/themes/teams/components/Alert/alertStyles.ts 8% <0%> (-0.34%) ⬇️
...kages/react/src/themes/teams-dark/siteVariables.ts 100% <0%> (ø) ⬆️
...ms/components/Header/headerDescriptionVariables.ts 0% <0%> (ø) ⬆️
...ams-high-contrast/components/Menu/menuVariables.ts 0% <0%> (ø) ⬆️
...src/themes/teams/components/Menu/menuItemStyles.ts 7.14% <0%> (ø) ⬆️
...act/src/themes/teams/components/Text/textStyles.ts 0% <0%> (ø) ⬆️
...rc/themes/teams/components/Label/labelVariables.ts 0% <0%> (ø) ⬆️
packages/react/src/themes/base/colors.ts 100% <0%> (ø) ⬆️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31fa35e...c40b705. Read the comment docs.

@layershifter layershifter changed the title Popup experiment fix(Popup): avoid double rendering May 7, 2019
@layershifter layershifter marked this pull request as ready for review May 7, 2019 11:35
@layershifter layershifter added 🧰 fix Introduces fix for broken behavior. 🚀 ready for review labels May 7, 2019
…p/poup

# Conflicts:
#	CHANGELOG.md
#	packages/react/src/components/Popup/Popup.tsx
Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

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

👍 would be great to get this merged soon so I can reuse it for Dropdown #1294

@layershifter layershifter merged commit 1bf39a4 into master May 9, 2019
@delete-merged-branch delete-merged-branch bot deleted the exp/poup branch May 9, 2019 17:48
bmdalex pushed a commit that referenced this pull request May 9, 2019
* Popup experiment

* cleanup

* more cleanups

* rename file

* add changelog entries

* add "implements PopperJS.ReferenceObject"

* replace `triggerDomElement` with `triggerRef.current`

* update typings

* rewrite to WeakMap

* update comment

* remove memoization
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants