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

feat(Popper): made inline logic internal to Popper #8669

Merged
merged 4 commits into from
Feb 10, 2023

Conversation

thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented Feb 8, 2023

What: Closes #8673

Changes needed from the current PRs:

  • Date picker (#8636)
    • remove the inputGroupRef from line 123
    • Remove getInlineParentElement or revert it to the original getParentElement on line 201 (depends if we find it necessary anymore; if we revert it then "parent" needs to be added back to the prop types)
    • update line 246 where appendTo is passed a value; if we revert line 201 then this line can also be reverted, otherwise we can just make it appendTo={appendTo}
    • Remove the innerRef on line 250
  • LabelGroup demo (#8630)
    • Remove the appendTo from line 121
  • Nav flyout (#8628) is good as-is. Moving the popper menu contents to inside the <li> element will require investigating how popper-core sets the transform: translate property.
  • Popover (#8621) can mostly be reverted, but keep the default value as appendTo = "inline"
  • Composable menu demos (#8623): remove the appendTo prop from the examples
Original comment explaining the motivation

Essentially makes the logic for "inline" popper internal to Popper itself, rather than the components using Popper (Tooltip, Popover, Date Picker, etc) needing to house that logic (sometimes in different ways). For the most part this would only require another component to pass appendTo="inline" to Popper rather than needing to use a ref or call a function to determine it.

Using Popover as an example, the markup from this setup looks like this (using the children prop):

Popper contents after trigger wrapper

Or like this (using reference, though the resulting markup would depend if there's any other content between the trigger and the <Popover>):

Popper contents after trigger reference

I did try a slightly different setup locally, where if appendTo === "inline" then the popper contents would render inside of the trigger's wrapper <div style="display: contents"..., so might depend if anyone has a preference. The markup from this other approach ended up like this using the children prop on Popover:

Popper contents inside trigger wrapper

The Nav Flyout would be an outlier in that it either would need the logic implemented in #8628, or moving the flyout menu inside of the <li> within NavItem (doing this would require looking into how the transform: translate styling is applied to Popper via computedStyles in popper-core).

Additional issues:

@thatblindgeye
Copy link
Contributor Author

@patternfly-build
Copy link
Contributor

patternfly-build commented Feb 8, 2023

@thatblindgeye thatblindgeye marked this pull request as ready for review February 9, 2023 20:35
@nicolethoen nicolethoen merged commit 8824534 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.

Popper - make "inline" logic internal to Popper
5 participants