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

feat(dropdown): align, position, offset props + automatic position #1312

Merged
merged 38 commits into from
May 29, 2019

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented May 9, 2019

feat(dropdown): align, position, offset props + automatic position

BREAKING CHANGES MITIGATION

Replace Dropdown variables:

  • borderRadius ➡️ containerBorderRadius
  • openBorderRadius ➡️ openAboveContainerBorderRadius and openBelowContainerBorderRadius
  • listBorderRadius ➡️ aboveListBorderRadius and belowListBorderRadius

This PR tackles:

  • introducing align, position, offset props for Dropdown component
  • automatic positioning for Dropdown results
  • reuses code from fix(Popup): avoid double rendering #1153 to avoid double render and simplify code

Screen Recording 2019-05-10 at 18 05 46

TODO:

  • Refactor method names and clean code
  • Extract common code for positioning elements to src/lib/positioner
  • Integrate new Positioner helper component to Popup and Dropdown
  • Refactor unit tests for positionerHelper (move from Popup-test.tsx to dedicated file)
  • Figure out way how to add some extra padding between Dropdown trigger and list of results
    any ideas?

@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #1312 into master will increase coverage by 0.01%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1312      +/-   ##
==========================================
+ Coverage   73.57%   73.59%   +0.01%     
==========================================
  Files         787      787              
  Lines        5885     5888       +3     
  Branches     1734     1717      -17     
==========================================
+ Hits         4330     4333       +3     
  Misses       1549     1549              
  Partials        6        6
Impacted Files Coverage Δ
packages/react/src/lib/positioner/Popper.tsx 81.81% <ø> (+3.03%) ⬆️
...mes/teams/components/Dropdown/dropdownVariables.ts 50% <ø> (ø) ⬆️
...themes/teams/components/Dropdown/dropdownStyles.ts 13.88% <0%> (-2.78%) ⬇️
...ackages/react/src/components/Dropdown/Dropdown.tsx 88.57% <100%> (+0.05%) ⬆️
...ages/react/src/lib/positioner/positioningHelper.ts 100% <100%> (ø) ⬆️
...-contrast/components/Dropdown/dropdownVariables.ts 50% <100%> (+50%) ⬆️

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 c69313c...e23e956. Read the comment docs.

Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

The dropdown is jumping during page scroll:

dropdown_scroll

@bmdalex
Copy link
Collaborator Author

bmdalex commented May 13, 2019

The dropdown is jumping during page scroll:

@miroslavstastny I am aware of that. I was debugging popper code and noticed they recreate the popper instance (responsible for calculating styles that need to be applied for positioning elements) almost (they use debounce) every time there is a scroll/resize event: https://github.com/FezVrasta/popper.js/blob/36cc6c66e55f716232166c9a5725d8d08c4ec338/packages/popper/src/utils/setupEventListeners.js#L38 . This is where the jump comes from.

I think this can cause perf issues especially when trying to:

  • position complex elements (such as the list of results in Dropdown);
  • position multiple elements in the same view.

The jump is quite visible in our docs because there is a popper instance for every Dropdown example even if they are closed. This is due to the fact that for Dropdown we render the list of results even if it's empty (accessibility reasons, correct me if I'm wrong @silviuavram @jurokapsiar ).

MITIGATION:

I think I was able to mitigate this by adding eventsEnabled={open} prop to Popper, which means the scroll/resize events are attached only when the list of results is open for a Dropdown. @miroslavstastny can you please test again?

ALTERNATIVE:

I'm starting to think the simpler solution for Dropdown would be just to use position: 'absolute' and top/bottom CSS properties to position the the list of results above or below the trigger.

PROS:

  • no jump;
  • no perf issues because we won't recalculate anything on subsequent renders/scroll events/resize events;
  • no need to integrate with popper at all.

CONS:

  • we'd be limiting the Dropdown positioning to above or below the trigger; @jurokapsiar @silviuavram @layershifter would that be enough according to our specs? I didn't really see too many libraries aiming for more than this;
  • no auto positioning: we will need a function to tell us if the list of results is in the viewport to achieve that.

@miroslavstastny
Copy link
Member

  • no auto positioning: we will need a function to tell us if the list of results is in the viewport to achieve that.

https://react.semantic-ui.com/behaviors/visibility/

@bmdalex bmdalex force-pushed the feat/dropdown-position branch 3 times, most recently from 010ba8e to 27c507a Compare May 15, 2019 12:36
if (process.env.NODE_ENV !== 'production') {
if (knobs[knob.name]) {
throw new Error(`Knob with name "${knob.name}" has been already registered`)
setKnobs(prevKnobs => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix for the problem with using a different names for the same knob, thx @layershifter

Copy link
Member

Choose a reason for hiding this comment

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

This breaks existing docsite pages with knobs (Button, Menu):
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reverted so that I get unblocked, thanks, @layershifter let's tackle this fix in separate PR as the one proposed introduces the issue @miroslavstastny is mentioning

@bmdalex
Copy link
Collaborator Author

bmdalex commented May 23, 2019

The dropdown is jumping during page scroll:

dropdown_scroll

this perf issue was fixed by #1358

Alexandru Buliga and others added 4 commits May 29, 2019 10:34
@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

Merging #1312 into master will increase coverage by 0.01%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1312      +/-   ##
==========================================
+ Coverage   73.57%   73.59%   +0.01%     
==========================================
  Files         787      787              
  Lines        5885     5888       +3     
  Branches     1734     1717      -17     
==========================================
+ Hits         4330     4333       +3     
  Misses       1549     1549              
  Partials        6        6
Impacted Files Coverage Δ
packages/react/src/lib/positioner/Popper.tsx 81.81% <ø> (+3.03%) ⬆️
...mes/teams/components/Dropdown/dropdownVariables.ts 50% <ø> (ø) ⬆️
...themes/teams/components/Dropdown/dropdownStyles.ts 13.88% <0%> (-2.78%) ⬇️
...ackages/react/src/components/Dropdown/Dropdown.tsx 88.57% <100%> (+0.05%) ⬆️
...ages/react/src/lib/positioner/positioningHelper.ts 100% <100%> (ø) ⬆️
...-contrast/components/Dropdown/dropdownVariables.ts 50% <100%> (+50%) ⬆️

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 c69313c...e23e956. Read the comment docs.

@bmdalex bmdalex merged commit 727e9c2 into master May 29, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/dropdown-position branch May 29, 2019 13:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants