Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Popover): updated appendTo to inline by default #8621

Merged
merged 5 commits into from
Feb 10, 2023

Conversation

thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented Jan 30, 2023

What: Closes #8599

Popover examples

This results in markup similar to Timepicker (wrapping the Popper in a div internally and appending to that wrapper div).

Additional issues:

@patternfly-build
Copy link
Contributor

patternfly-build commented Jan 30, 2023

@thatblindgeye thatblindgeye force-pushed the iss8599_popoverAppendTo branch from 1ae1055 to f443fd7 Compare February 2, 2023 14:06
@thatblindgeye thatblindgeye force-pushed the iss8599_popoverAppendTo branch from f443fd7 to 4c993df Compare February 7, 2023 20:56
Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

@thatblindgeye does this need some sort of codemod/warning that there is now an extra div if appendTo was not explicitly set?

Oh, and suddenly another thought. What if people want to put a popover on something that is inline, like a button within a span? then they cannot append inline since it would result in a div being placed in a span. I wonder if the wrapping component should also be something that the user can configure to ensure valid HTML?

@thatblindgeye
Copy link
Contributor Author

@nicolethoen I can add that in to the codemod PR I currently have up!

Regarding configuring the wrapper component, that might require further updating elements of the Popover since Popover uses divs internally in several places, so no matter what a div may end up within a span.

In terms of that extraneous div, though, would it make sense to have the logic for placing elements inline placed inside Popper instead? I tweaked the Popper logic locally and it seems like it may result in the outcome we want in most cases.

Copy link
Contributor

@gitdallas gitdallas left a comment

Choose a reason for hiding this comment

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

small miss

packages/react-core/src/components/Popover/Popover.tsx Outdated Show resolved Hide resolved
@thatblindgeye thatblindgeye force-pushed the iss8599_popoverAppendTo branch from 839d574 to 6b31199 Compare February 8, 2023 15:53
@thatblindgeye thatblindgeye force-pushed the iss8599_popoverAppendTo branch from 6b31199 to e6222bc Compare February 10, 2023 18:36
@nicolethoen nicolethoen merged commit f6522ca into patternfly:v5 Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popover's appendTo prop defaults to the document body. it should default to inline.
5 participants