feat(Popover): Add PF4 Popover

[boaz0]

closes #487

[coveralls]

Pull Request Test Coverage Report for Build 3328

• 26 of 26 (100.0%) changed or added relevant lines in 7 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.07%) to 81.394%

---

Totals
Change from base Build 3325:	0.07%
Covered Lines:	3902
Relevant Lines:	4517

---

💛 - Coveralls

[patternfly-build]

PatternFly-React preview: https://754-pr-patternfly-react-patternfly.surge.sh

[mcarrano]

This looks good @boaz1337 But just a couple of comments:

• Clicking outside of the popover (as well as the Close button) should toggle it closed. See the PF3 React component. The behavior should be the same.
• The Header should not be a required element. I can envision cases where I would just want to place a message inside a popover without including a title or header.

[jschuler]

Hi @boaz1337 , thanks for taking a stab at this. Few things, @mcarrano please correct me if I am wrong:
The popover should be able to 'pop' out of the constraints of its parent container. It should basically float on top and not be bounded to or by the layout. So it shouldn't also expand the container it is sitting in to make space for itself when shown.

Also from what I can tell in this example, the popover position is dependent on where it is in the DOM. The position prop sets the arrow in the right place, but the popover is not positioned correctly relative to an element (e.g. the button).

Because of this we have suggested incorporating popper.js thidparty library in #487 to help with positioning.

[mcarrano]

@jschuler Yes, you are correct. The popover is an element that floats on top of other content and is useful for things like displaying info tips on forms, etc.

[boaz0]

@mcarrano - thanks I will update the props - header is no longer required.
@jschuler thanks for the reference, I tried to use react-popper but I got a weird error when running the docs. I'm now figuring what is the problem and hopefully update this PR soon.

Thanks!

[boaz0]

I updated the examples + changed the props as suggested.
I am working now on integrating react-popper - unfortunately, the documents are not very clear: doing a lot of trial and error.

[patternfly-build]

Deploy preview for patternfly-react-gone ready!

Built with commit 2096d00

https://deploy-preview-754--patternfly-react-gone.netlify.com

[christiemolloy]

Thanks for implementing the Popover @boaz1337 it looks great!! I have some comments for the CSS/UX review.

I believe we want to keep the content(text) inside of the popover component the same as the content that we have inside of pf-next to keep consistency. For example, swapping out "Popover title" to "Popover Header" and the same for the body.

I really like the option to change where the arrow should be positioned relative to the box. What is the plan for where the dropdown select will go? The user who implements the React component should change the arrow position within the code, not on the page. Otherwise the end user would also be able to change this. So I believe we wont need the dropdown implementation with the popover.

Are you planning to add implementation to the close button, enabling the user to exit out of the popover? That would be great to see :) I also wonder how the popover will pop up from text. @matthewcarleton I know you are looking at animation right now, would this be a good example where animation is needed?

The design for the headless popover looks like it has alignment issues. I will circle in a designer for a review on that so that we can make this a possibility. Again we would need to see this in PF-Next before we implement it in PF-React, but this is something we can do.

Circling in @mcarrano for a review on the Customized Popover (with arrow-less and no option to close button). What are your thoughts on this variations? Do they align with what we originally had as variations in the designs for the popover?

This looks great, let me know if you have any questions!

[mcarrano]

Per our discussion, let's remove the Customized Popover variations. We can add these later if a use case comes up that requires them. The Popover without Header requires some styling updates but we will do those in Core first and apply here.

[rachael-phillips]

Hi @boaz1337! Are you interesting in continuing your work on this?

[boaz0]

Hi @rachael-phillips

Sorry that it took me too long - I will be working on this in the next days.

[rachael-phillips]

Ok great! Thanks!

[boaz0]

@mcarrano & @christiemolloy can you PTAL.
Let me know if I'm missing anything.

[mcarrano]

@boaz1337 This is looking better, but just a few comments:

• I wasn't sure why the first two examples were different. Can they be combined? It seems like the variables are the position of the popover and whether it has an explicit Close button.
• Besides clicking the close (X) button, a popover should dismiss if the user clicks outside of it.
• The formatting in the third example, headless popover is not quite right. Please refer to the core example here: https://pf-next.netlify.com/components/Popover/examples/

[boaz0]

Updated the Popover examples as you suggested.

However, it seems like the problem with the headless popover comes from patternfly-next.
For some reason, pf-c-popover__body set padding-top to 8 while it should be set to 32.
I will try to figure out how to fix this.

Thanks @mcarrano

[tlabaj]

@boaz1337 Core merged PR https://github.com/patternfly/patternfly-next/pull #837 that refactored Popover. Could you update this PR to reflect those changes?

[boaz0]

It looks like I can't update to patternfly-next v1.0.72.
On my machine the tests passed and the look of popover is fixed.

I think the problem is because on my machine react-styled-system isn't built.

Any thoughts?

[jschuler]

Hi @boaz1337 , every project that has @patternfly/patternfly-next in its package.json needs to have the same version number, that includes react-styled-system, react-charts, react-tokens, react-icons, and react-core

[jschuler]

Also, in order to have this be ready for consumers we need positioning engine (i.e. popper.js). But this is already great work here! I think we can either a) Merge this but not yet export the components out (in index.js), or b) Include positioning engine.

[boaz0]

Thanks @jschuler I rebased and updated package.json.

I am not sure my popper.js implementation is good enough. I would rather work on the engine in other PR. Unless, people think differently.

Thanks.

[tlabaj]

Can you update the examples so that they match the examples in PF-next.

[boaz0]

I updated the examples to match to PF next.
@tlabaj can you please take a look and let me know if I'm missing something.
Thanks a lot.

[matthewcarleton]

@christiemolloy yup! we definitely want to consider animation here. We are currently working out how we are going to tackle animation so it likely won't be ready for this PR but we will have this component on the list for future updates.

[tlabaj]

Thanks @boaz1337, the example updates look good!

[christiemolloy]

@boaz1337 this looks really good thanks for making changes! Will the Close button on the "headless" popover work?
CSS approved :)

[jschuler]

Hey @boaz1337 , I went ahead and merged from master and also removed the popover component from the export list. This way we can merge this code and not expose to the consumer. Once positioning work is ready we can add it back into the index.js files.

[jschuler]

@mcarrano @tlabaj Are you guys good with the changes?
Please use this URL to view the latest version
http://popover.surge.sh/components/popover

[mcarrano]

I approve this for now with the understanding that event handling will be integrated later via separate PR. The popover should dismiss whenever the user clicks outside of it as well as when clicking the Close button.

[tlabaj]

LGTM

[boaz0]

Thank you so much!