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

chore(composable menu demos): updated appendTo #8623

Merged

Conversation

thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented Jan 30, 2023

What: Closes #8600

Composable menu demos

Moved the <div ref={containerRef}> wrapper to the toggle, which is similar to the proposed solution for the "Editable labels with add dropdown" demo in the epic.

Alternatively since all of these demos are setup the same way, we could append to toggleRef.current.parent, which should always be the wrapper div provided by Popper with style="display: contents":

Menu composable dropdown markup

Additional issues:

@patternfly-build
Copy link
Contributor

patternfly-build commented Jan 30, 2023

@nicolethoen
Copy link
Contributor

nicolethoen commented Feb 2, 2023

@thatblindgeye is there a reason we cannot append it to the toggle itself? It appears that if I append it to the toggleRef.current, it appears stylistically to work. But I haven't played with it extensively.

@thatblindgeye
Copy link
Contributor Author

is there a reason we cannot append it to the toggle itself? It appears that if I append it to the toggleRef.current, it appears stylistically to work. But I haven't played with it extensively.

Doing that ends up placing the menu inside the <button> element, so we end up with buttons/anchors inside of buttons.

@nicolethoen
Copy link
Contributor

riiiiight

@thatblindgeye thatblindgeye force-pushed the iss8600_menuDemosAppend branch from 5dfa77b to f8c6f25 Compare February 7, 2023 20:57
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.

Your alternate solution does sound pretty appealing. If all the demos already have a parent wrapping div, we can remove one level of div nesting - am I understanding that right?

@thatblindgeye
Copy link
Contributor Author

Your alternate solution does sound pretty appealing. If all the demos already have a parent wrapping div, we can remove one level of div nesting - am I understanding that right?

Yep! Appending to the toggle's parent results in the following markup:

Popper appended to toggle's parent

(quick note that my latest comment in my Popover PR could also apply to these demos as well)

@kmcfaul
Copy link
Contributor

kmcfaul commented Feb 8, 2023

Just a point of caution for the alternate solution as the wrapping div can be removed if the references are passed to Popper instead of the usual trigger + popper props. I believe all our components/examples use the typical method though, not references.

@thatblindgeye
Copy link
Contributor Author

Latest update based on 8669 linked above getting merged in first.

@thatblindgeye thatblindgeye force-pushed the iss8600_menuDemosAppend branch from 1b8b98c to 0fb481c Compare February 10, 2023 18:54
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.

This is beautiful 🥹

@wise-king-sullyman wise-king-sullyman merged commit b03c06e 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.

Composable menu demos should append Popper inline by default
5 participants