fix: Align popover inside dropdown #789

parostatkiem-zz · 2019-11-05T09:41:46Z

This is a temporary solution.

The whole problem requires a major refactoring on popovers, as @jbadan said on Slack (100% agree).
This is just a "patch" to make Popover working with Dropdown or any other element.
⚠️ It does not introduce any backwards-incompatible changes.

Description

Add ability to align Popover left and right edge with its parent control element edges.
Add ability to set Popover right edge to be not-further-left or not-further-right than parent control right edge.

It works well with Dropdown as well as any other type. ~~Sadly, the solution requires creating a reference on the client side.~~

Before:

After:

fixes #788

netlify · 2019-11-05T09:44:44Z

Check the deploy log for errors here: https://app.netlify.com/sites/fundamental-react/deploys

Built with commit ad7f0ed

https://app.netlify.com/sites/fundamental-react/deploys/5dd831e64b7c120008d946bc

markpalfreeman · 2019-11-05T20:08:23Z

src/Popover/Popover.js

 };

 Popover.propDescriptions = {
    body: 'Node(s) to render in the overlay.',
    control: 'Node to render as the reference element (that the `body` will be placed in relation to).',
    disableEdgeDetection: 'Set to **true** to render popover without edge detection so popover will not flip from top to bottom with scroll.',
    noArrow: 'Set to **true** to render a popover without an arrow.',
+    parentElement: 'A reference (useRef) to an element of which the Popover will be positioned and sized relatively. Leave it empty to let it be done automatically.',


Can we call this parentRef to be even more explicit?

And if we're going to mention useRef(), can we clarify the function itself shouldn't be passed in, but rather the return object from calling it?

Also, the description seems to imply ("let it be done") that positioning and sizing relative to its parent can happen automatically if left empty. Is that the case?

Sadly, it's not. If it was, this PR wouldn't need to exist at all ;)
I had no idea how to let the developer know he should put useRef's result there... Your idea of parentRef seems reasonable.

I tried to put some comments into the code displayed in the documentation website but it's impossible :(

markpalfreeman · 2019-11-05T20:09:01Z

src/utils/_Popper.js

@@ -94,6 +95,11 @@ class Popper extends React.Component {
                    if (!show) {
                        return null;
                    }
+                    const currentParentElement = parentElement && parentElement.current;


Regarding my other comment, parentRef would be slightly clearer here that we could access a property .current.

bcullman · 2019-11-05T20:27:48Z

@parostatkiem - can you add before and after screen caps?

parostatkiem-zz · 2019-11-06T08:55:08Z

@bcullman you can clearly see it live on the deployed preview.
https://deploy-preview-789--fundamental-react.netlify.com/dropdown

Open the Width limited to parent's dropdown, then open any other one. You'll see the difference :)

jbadan

This needs some unit tests added and screenshots - while I understand we can currently see it with the deploy preview, for history's sake if we need to look back it's better to have some screenshots in the description.

parostatkiem-zz · 2019-11-07T09:11:39Z

@jbadan ✅done
I couldn't come up with any more tests scenarios...

bcullman · 2019-11-14T18:55:22Z

@parostatkiem - I could imagine a better long term fix would be to address this here by adding a opt-in prop for `widthSizingType' here:

https://github.com/SAP/fundamental-react/blob/master/src/utils/_Popper.js

that accepted the following

    ‘none’,
   ‘matchTarget’,
   ‘minTarget’,
   ‘maxTarget’

it would fix this component, and be flexible enough to handle all other circumstances, including other copmponets relying on _popper.js

parostatkiem-zz · 2019-11-15T08:44:47Z

@bcullman - where it would take the target's width from? The problem is Popper has no "idea" of his parent. That's why I introduced parentRef which has an obvious disadvantage of being defined on the client side.

... or do you mean adding widthSizingType on top of my solution? If yes, that would mean if widthSizingType has any other value than none, parentRef would have to be defined.

greg-a-smith · 2019-11-19T16:54:38Z

@parostatkiem The _Popper.js component has a prop called referenceComponent, which is the element to be sized in relation to.

…o align-popover-inside-dropdown # Conflicts: # src/Popover/Popover.js

parostatkiem-zz · 2019-11-20T13:40:18Z

@greg-a-smith it was the first place I tried to find a solution at but I couldn't get the real DOM node of the referenceComponent, so I abandoned the idea and did it the other way.
However, today you inspired me to give it another chance and... here it is 🍾
Thank you for the inspiration!

@bcullman I implemented your idea in an unchanged form. I finally got rid of a ref on the client side :)

greg-a-smith

There appears to be some display issues with the popover examples using the Menu component, but I think that's another issue altogether. Not sure why Menu is a display type of table, but it is. 🤔 This is part of the reason why new Select and List components were built in fundamental-styles. The down side is they haven't been created in fundamental-react yet. 😞 Let's go forward with these changes for now and revisit if and when necessary. 🚢

jbadan · 2019-11-22T23:03:01Z

@parostatkiem I tried to resolve some conflicts for you here, but I can't push to your branch to fix the failing test. Ping the team when the tests pass and we will merge :)

parostatkiem-zz · 2019-11-22T23:07:49Z

@jbadan I added you to collaborators, you should receive an invitation email.
I'm quite busy this night (it's midnight right now) and will be able to fix the tests on Monday at the earliest.

jbadan · 2019-11-26T17:48:38Z

closing for #804

parostatkiem-zz added 4 commits November 4, 2019 09:32

Add 'parentElement' prop to Popover

a969767

Add semicolon

fb90a79

Update solution

162731c

update documentation

1e80fe4

jbadan requested a review from a team November 5, 2019 19:53

jbadan changed the title ~~Align popover inside dropdown~~ fix: Align popover inside dropdown Nov 5, 2019

markpalfreeman reviewed Nov 5, 2019

View reviewed changes

parostatkiem-zz added 2 commits November 6, 2019 10:20

Change example list items

4de9463

rename parentElement to parentRef

0ffcaa0

parostatkiem-zz force-pushed the align-popover-inside-dropdown branch from 1f61f0e to 0ffcaa0 Compare November 6, 2019 10:17

jbadan requested changes Nov 6, 2019

View reviewed changes

Add unit test

9cf7df1

parostatkiem-zz added 2 commits November 20, 2019 14:24

Refactor the solution to use placementTargetRef and widthSizingType

78d2687

Merge branch 'master' of https://github.com/SAP/fundamental-react int…

83ca8f1

…o align-popover-inside-dropdown # Conflicts: # src/Popover/Popover.js

greg-a-smith approved these changes Nov 21, 2019

View reviewed changes

jbadan approved these changes Nov 21, 2019

View reviewed changes

jbadan mentioned this pull request Nov 21, 2019

fix: Align popover inside dropdown #798

Closed

Merge branch 'master' into align-popover-inside-dropdown

ad7f0ed

jbadan mentioned this pull request Nov 26, 2019

fix: align popover inside dropdown #804

Merged

jbadan closed this Nov 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Align popover inside dropdown #789

fix: Align popover inside dropdown #789

parostatkiem-zz commented Nov 5, 2019 •

edited

Loading

netlify bot commented Nov 5, 2019 •

edited

Loading

markpalfreeman Nov 5, 2019

markpalfreeman Nov 5, 2019

parostatkiem-zz Nov 6, 2019

markpalfreeman Nov 5, 2019

bcullman commented Nov 5, 2019

parostatkiem-zz commented Nov 6, 2019

jbadan left a comment

parostatkiem-zz commented Nov 7, 2019 •

edited

Loading

bcullman commented Nov 14, 2019 •

edited

Loading

parostatkiem-zz commented Nov 15, 2019 •

edited

Loading

greg-a-smith commented Nov 19, 2019

parostatkiem-zz commented Nov 20, 2019 •

edited

Loading

greg-a-smith left a comment

jbadan commented Nov 22, 2019

parostatkiem-zz commented Nov 22, 2019

jbadan commented Nov 26, 2019

fix: Align popover inside dropdown #789

fix: Align popover inside dropdown #789

Conversation

parostatkiem-zz commented Nov 5, 2019 • edited Loading

This is a temporary solution.

Description

Before:

After:

netlify bot commented Nov 5, 2019 • edited Loading

markpalfreeman Nov 5, 2019

Choose a reason for hiding this comment

markpalfreeman Nov 5, 2019

Choose a reason for hiding this comment

parostatkiem-zz Nov 6, 2019

Choose a reason for hiding this comment

markpalfreeman Nov 5, 2019

Choose a reason for hiding this comment

bcullman commented Nov 5, 2019

parostatkiem-zz commented Nov 6, 2019

jbadan left a comment

Choose a reason for hiding this comment

parostatkiem-zz commented Nov 7, 2019 • edited Loading

bcullman commented Nov 14, 2019 • edited Loading

parostatkiem-zz commented Nov 15, 2019 • edited Loading

greg-a-smith commented Nov 19, 2019

parostatkiem-zz commented Nov 20, 2019 • edited Loading

greg-a-smith left a comment

Choose a reason for hiding this comment

jbadan commented Nov 22, 2019

parostatkiem-zz commented Nov 22, 2019

jbadan commented Nov 26, 2019

parostatkiem-zz commented Nov 5, 2019 •

edited

Loading

netlify bot commented Nov 5, 2019 •

edited

Loading

parostatkiem-zz commented Nov 7, 2019 •

edited

Loading

bcullman commented Nov 14, 2019 •

edited

Loading

parostatkiem-zz commented Nov 15, 2019 •

edited

Loading

parostatkiem-zz commented Nov 20, 2019 •

edited

Loading