Skip to content

fix(selector-dropdown): add dynamic positioning to the dropdown overlay - #3262

Merged
mergify[bot] merged 8 commits into
masterfrom
popper-selector-dropdown
Mar 21, 2023
Merged

mergify[bot] merged 8 commits into
masterfrom
popper-selector-dropdown

Conversation

@ivanthai

@ivanthai ivanthai commented Feb 24, 2023 •

Copy link
Copy Markdown
Contributor

Updates

  • Added explanation of the design decision of adding the isPopperEnabled prop to preserve the former SelectorDropdown behavior
  • Added forwardRef to SearchForm because it is used as a child to PopperComponent to SelectorDropdown

Overview

SelectorDropdown is not responsive in terms of the dropdown positioning. i.e. when the dropdown doesn't have enough space between the triggering element and the bottom viewport, it doesn't render above the triggering element instead. This change was needed because there is at least one element that doesn't render well in small viewports

Solution

Utilize react-popper to handle the rendering of the overlay. This required adding forwardRefs to the respective children components. forwardRef and flow don't get along together well so a few

To allow backward compatibility with other components that do not want to use react-popper which would change the overlay to use use absolute positioning, isPopperEnabled was added. This way, components like TemplateDropdown can remain compatible with SelectorDropdown. TemplateDropdown embeds a PopperComponent within a Flyout which would essentially be embedding an absolute within an absolute without a defined height.

An alternative to adding the isPopperEnabled prop is to have TemplateDropdown override the behavior using CSS which was a bit simplier, but more hacky

// TemplateDropdown.scss
.metadata-instance-editor-template-dropdown-menu {
    .overlay-wrapper {
        // override react-popper
        position: relative ! important;
        transform: none ! important;
...

Demo

Issue - dropdown renders below the viewport
image

Fix - dropdown renders above the triggering element when there is no space
content-filter-owners-search-fix

Smallest supported viewport
image

@ivanthai
ivanthai force-pushed the popper-selector-dropdown branch from 286394b to 81416cd Compare February 24, 2023 02:06
@ivanthai
ivanthai force-pushed the popper-selector-dropdown branch from 6ea35eb to e3e6702 Compare February 24, 2023 02:24
@ivanthai
ivanthai force-pushed the popper-selector-dropdown branch 3 times, most recently from a0e62b6 to c541977 Compare February 28, 2023 02:27
@ivanthai
ivanthai marked this pull request as ready for review February 28, 2023 02:55
@ivanthai
ivanthai requested review from a team as code owners February 28, 2023 02:55
@ivanthai
ivanthai force-pushed the popper-selector-dropdown branch from c541977 to b64ada0 Compare February 28, 2023 02:59
Comment thread src/components/pill-selector-dropdown/PillSelector.js Outdated
Comment thread src/components/selector-dropdown/SelectorDropdown.js Outdated
@ivanthai
ivanthai force-pushed the popper-selector-dropdown branch 4 times, most recently from acaf55d to 6c15b85 Compare March 1, 2023 02:17
@ivanthai
ivanthai requested a review from greathmaster March 1, 2023 04:04
greathmaster
greathmaster previously approved these changes Mar 1, 2023
Comment thread src/components/pill-selector-dropdown/PillSelector.js
Comment thread src/components/selector-dropdown/SelectorDropdown.js Outdated
Comment thread src/components/selector-dropdown/SelectorDropdown.scss
@ivanthai
ivanthai requested a review from a team as a code owner March 1, 2023 22:59
@ivanthai
ivanthai force-pushed the popper-selector-dropdown branch 3 times, most recently from 11fd45a to 977c5a1 Compare March 1, 2023 23:55
Comment thread src/features/metadata-instance-editor/TemplateDropdown.js
@ivanthai
ivanthai force-pushed the popper-selector-dropdown branch from 1b33ee4 to a21966c Compare March 2, 2023 18:23
@ivanthai
ivanthai force-pushed the popper-selector-dropdown branch from 24d8fa6 to 11e0847 Compare March 8, 2023 01:47
Comment thread src/components/conditional-wrapper/ConditionalWrapper.js Outdated
Comment thread src/components/conditional-wrapper/ConditionalWrapper.js Outdated
Comment thread src/components/conditional-wrapper/__tests__/ConditionalWrapper.test.js Outdated
Comment thread src/components/conditional-wrapper/__tests__/ConditionalWrapper.test.js Outdated
Comment thread src/components/selector-dropdown/SelectorDropdown.scss Outdated
Comment thread src/components/selector-dropdown/SelectorDropdown.js Outdated
@ivanthai
ivanthai requested a review from tjuanitas March 14, 2023 19:31
Machoper
Machoper previously approved these changes Mar 15, 2023
Comment thread src/components/popper/PopperComponent.js Outdated
Comment thread src/components/selector-dropdown/SelectorDropdown.js
Comment thread src/components/selector-dropdown/SelectorDropdown.js Outdated
Comment thread src/components/pill-selector-dropdown/PillSelector.js
Comment thread src/components/pill-selector-dropdown/PillSelector.js Outdated
Comment thread src/components/pill-selector-dropdown/PillSelector.js
Comment thread src/components/search-form/SearchForm.js Outdated
tjuanitas
tjuanitas previously approved these changes Mar 20, 2023

@tjuanitas tjuanitas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

left a question but looks good to me!

Comment thread src/components/pill-selector-dropdown/PillSelector.scss
Comment thread src/components/search-form/SearchForm.js Outdated
@tjuanitas

Copy link
Copy Markdown
Contributor

fyi I think some of the changes got reverted back in the last push

@ivanthai
ivanthai force-pushed the popper-selector-dropdown branch from 7e92839 to 2ce809f Compare March 21, 2023 18:33

@JChan106 JChan106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@greg-in-a-box
greg-in-a-box removed the request for review from greathmaster March 21, 2023 22:27
@mergify
mergify Bot merged commit 7f05653 into master Mar 21, 2023
@mergify
mergify Bot deleted the popper-selector-dropdown branch March 21, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants