feat(dropdown): use Popper.js for a proper positioning #597

denkristoffer · 2020-10-20T14:04:05Z

Purpose of PR

This PR refactors the Dropdown component to use Popper.js instead of the current, custom positioning calculation.

Resolves #361

Todo

Dynamic content
Don't re-open when toggling on the toggleElement
~~What to do about the isAutoalignmentEnabled prop? It doesn't serve much of a purpose with Popper doing the positioning automatically. Is it considered a breaking change to remove it?~~

PR Checklist

I have read the relevant readme.md file(s)
All commits follow our Git commit message convention
Tests are added/updated/not required
Tests are passing
Storybook stories are added/updated/not required
Usage notes are added/updated/not required
Has been tested based on Contentful's browser support
Doesn't contain any sensitive information

netlify · 2020-10-20T14:25:15Z

Deploy preview for forma-36-react-components ready!

Built with commit 20880fb

https://deploy-preview-597--forma-36-react-components.netlify.app

netlify · 2020-10-20T14:54:37Z

Deploy preview for forma-36-react-components ready!

Built with commit 4651b47

https://deploy-preview-597--forma-36-react-components.netlify.app

m10l · 2020-10-21T08:40:55Z

Awesome 😻, looks like there are some issues with the react-timepicker package though due to React mismatch?

lerna ERR! yarn run test exited 1 in '@contentful/[secure]-react-timepicker'
lerna ERR! yarn run test stdout:
$ tsdx test --env=jsdom
  console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29
    Error: Uncaught [Error: Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:
    1. You might have mismatching versions of React and the renderer (such as React DOM)
    2. You might be breaking the Rules of Hooks
    3. You might have more than one copy of React in the same app
    See https://fb.me/react-invalid-hook-call for tips about how to debug and fix this problem.]

m10l

Awesome work

gui-santos

🚀

gui-santos · 2020-10-22T14:03:50Z

...es/forma-36-react-components/src/components/Dropdown/DropdownContainer/DropdownContainer.tsx

-      styles[`DropdownContainer__container-position--${this.state.position}`],
-    );
+      return () => {
+        document.body.removeChild(portalTarget.current);


Maybe we should refactor the Tooltip to use Portals as well. But In another PR of course 😄

burakukula · 2020-10-22T15:44:46Z

packages/forma-36-react-components/src/components/Dropdown/Dropdown.stories.tsx

@@ -18,7 +18,6 @@ function DefaultStory() {
      onClose={() => setOpen(false)}
      isFullWidth={boolean('isFullWidth', false)}
      key={Date.now()} // Force Reinit
-      isAutoalignmentEnabled={boolean('isAutoalignmentEnabled', true)}


We might want to refactor it to the new format, but we can totally do it later.

burakukula

🎉 Great work!
As you are removing isAutoalignmentEnabled would this mean minor change in the API?
On a separate PR we could do refactor to the story to new storybook format and add README for the component. But we can totally do it later.
Overall great! 🙌

DanweDE

As you are removing isAutoalignmentEnabled would this mean minor change in the API?

This is kind of a breaking change in the sense that previously you could disable auto-alignment explicitly via isAutoalignmentEnabled={false}. If you rely on this behavior for some reason it's breaking for you.

@denkristoffer Searching for "isAutoalignmentEnabled" I noticed that we're relying on this in the web app in two places. If you really want to remove this prop, please make sure it's not strictly necessary there and that the UI around it still works fine without it.

burakukula · 2020-10-23T09:26:11Z

@denkristoffer if this means breaking change, I think we should w8 and group this with other changes to release bigger major version. We have couple of things - We could move some of the alpha components to main build I think and I'm sure that we have couple of more things that might do into the next major release. Maybe a catch up regarding this topic would be a good thing to do? WDYT?

denkristoffer · 2020-10-23T15:59:29Z

@burakukula @DanweDE I think I managed to replicate disabling positioning with the isAutoAlignmentEnabled prop. I have un-deprecated it, so we can get this in without breaking changes. I think it's ready.

mshaaban0 · 2020-10-23T18:11:43Z

...es/forma-36-react-components/src/components/Dropdown/DropdownContainer/DropdownContainer.tsx

-    const { submenu, className, width, testId } = this.props;
+    useEffect(() => {
+      if (isOpen) {
+        document.body.appendChild(portalTarget.current);


@denkristoffer if you can find a way to not to use portal it would be awesome. the problem with portal is keyboard focus order; upon open the dropdown content is at the end of the document, onClose the next keyboard element to be focused is the first element in the document. I hope that makes any sense, let's talk about it if not :)

@mshaaban0 I agree, the portal should be removed by default. I think we should handle this separately in #601 though, also to make sure this PR doesn't break any existing uses of the dropdown. Is that okay with you?

burakukula · 2020-10-26T10:06:38Z

@denkristoffer ok, cool. 😎
Sorry, maybe it's a silly question. I think the prop isAutoalignmentEnabled is still removed from the API in the PR? Or I am missing something? It's very possible that I am missing something. Hope you don't mind me asking. 🙃
I'm thinking about potential usage of the API outside of the webapp. if someone rely on this, it will still be breaking change?

denkristoffer · 2020-10-26T10:40:46Z

@burakukula Of course not 🙂 The prop is being used in the Dropdown component where Popper is used, instead of the DropdownContainer where it was used before: https://github.com/contentful/forma-36/blob/dropdown-popper/packages/forma-36-react-components/src/components/Dropdown/Dropdown.tsx#L149-L164

I've also added it back to the story.

burakukula · 2020-10-26T12:31:26Z

@denkristoffer super, thx! (the file was folded from me 😑 and I didn't see this change. sorry for confusion)

DanweDE · 2020-10-26T12:59:28Z

...es/forma-36-react-components/src/components/Dropdown/DropdownContainer/DropdownContainer.tsx

-  testId: 'cf-ui-dropdown-portal',
-  position: 'bottom-left',
-  submenu: false,
-  isAutoalignmentEnabled: true,


@denkristoffer Since you made the prop work again, I think you'd want to keep the default value of true for this prop and also add it back to the DropdownContainerProps interface above, no?

The functionality has been moved to Dropdown.tsx, so you shouldn't ever want/have to pass it to DropdownContainer.

denkristoffer force-pushed the dropdown-popper branch from 9a1bc38 to d583c96 Compare October 21, 2020 08:06

This comment has been minimized.

Sign in to view

denkristoffer force-pushed the dropdown-popper branch 2 times, most recently from 4dd2a37 to f31379e Compare October 21, 2020 14:02

giotiskl requested a review from m10l October 21, 2020 14:31

m10l approved these changes Oct 22, 2020

View reviewed changes

denkristoffer force-pushed the dropdown-popper branch from 6f03ad8 to 4cb1c5a Compare October 22, 2020 12:52

denkristoffer marked this pull request as ready for review October 22, 2020 12:53

denkristoffer requested review from burakukula, gui-santos and mshaaban0 as code owners October 22, 2020 12:53

m10l mentioned this pull request Oct 22, 2020

feat: 🎸 auto position combined link actions dropdown contentful/field-editors#453

Merged

gui-santos approved these changes Oct 22, 2020

View reviewed changes

burakukula reviewed Oct 22, 2020

View reviewed changes

denkristoffer force-pushed the dropdown-popper branch from 4cb1c5a to 213ecf0 Compare October 22, 2020 15:57

DanweDE reviewed Oct 22, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

denkristoffer force-pushed the dropdown-popper branch from 213ecf0 to 8a86e31 Compare October 23, 2020 15:57

mshaaban0 reviewed Oct 23, 2020

View reviewed changes

denkristoffer force-pushed the dropdown-popper branch 2 times, most recently from 6db8d89 to 77e3f65 Compare October 26, 2020 09:03

denkristoffer force-pushed the dropdown-popper branch from 77e3f65 to 14acbdc Compare October 26, 2020 10:36

refactor(dropdown): use Popper.js for positioning

114d0bb

denkristoffer force-pushed the dropdown-popper branch from 14acbdc to 114d0bb Compare October 26, 2020 10:57

burakukula approved these changes Oct 26, 2020

View reviewed changes

DanweDE reviewed Oct 26, 2020

View reviewed changes

Merge branch 'master' into dropdown-popper

4651b47

suevalov changed the title ~~refactor(dropdown): Use Popper.js for positioning~~ feat(dropdown): use Popper.js for a proper positioning Oct 26, 2020

suevalov merged commit 67ec949 into master Oct 26, 2020

denkristoffer deleted the dropdown-popper branch October 27, 2020 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dropdown): use Popper.js for a proper positioning #597

feat(dropdown): use Popper.js for a proper positioning #597

denkristoffer commented Oct 20, 2020 •

edited

Loading

netlify bot commented Oct 20, 2020

netlify bot commented Oct 20, 2020 •

edited

Loading

m10l commented Oct 21, 2020

This comment has been minimized.

m10l left a comment

gui-santos left a comment

gui-santos Oct 22, 2020

burakukula Oct 22, 2020

burakukula left a comment

DanweDE left a comment •

edited

Loading

burakukula commented Oct 23, 2020

This comment has been minimized.

denkristoffer commented Oct 23, 2020

mshaaban0 Oct 23, 2020

denkristoffer Oct 26, 2020

burakukula commented Oct 26, 2020 •

edited

Loading

denkristoffer commented Oct 26, 2020

burakukula commented Oct 26, 2020

DanweDE Oct 26, 2020

denkristoffer Oct 26, 2020

feat(dropdown): use Popper.js for a proper positioning #597

feat(dropdown): use Popper.js for a proper positioning #597

Conversation

denkristoffer commented Oct 20, 2020 • edited Loading

Purpose of PR

Todo

PR Checklist

netlify bot commented Oct 20, 2020

netlify bot commented Oct 20, 2020 • edited Loading

m10l commented Oct 21, 2020

This comment has been minimized.

m10l left a comment

Choose a reason for hiding this comment

gui-santos left a comment

Choose a reason for hiding this comment

gui-santos Oct 22, 2020

Choose a reason for hiding this comment

burakukula Oct 22, 2020

Choose a reason for hiding this comment

burakukula left a comment

Choose a reason for hiding this comment

DanweDE left a comment • edited Loading

Choose a reason for hiding this comment

burakukula commented Oct 23, 2020

This comment has been minimized.

denkristoffer commented Oct 23, 2020

mshaaban0 Oct 23, 2020

Choose a reason for hiding this comment

denkristoffer Oct 26, 2020

Choose a reason for hiding this comment

burakukula commented Oct 26, 2020 • edited Loading

denkristoffer commented Oct 26, 2020

burakukula commented Oct 26, 2020

DanweDE Oct 26, 2020

Choose a reason for hiding this comment

denkristoffer Oct 26, 2020

Choose a reason for hiding this comment

denkristoffer commented Oct 20, 2020 •

edited

Loading

netlify bot commented Oct 20, 2020 •

edited

Loading

DanweDE left a comment •

edited

Loading

burakukula commented Oct 26, 2020 •

edited

Loading