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

Use Floating UI for MenuItem submenus #6457

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

radium-v
Copy link
Collaborator

@radium-v radium-v commented Oct 14, 2022

Pull Request

📖 Description

Replaces the methods used to position submenus for menu-item with Floating UI.

Changes the updateSubmenu method to be public to allow for overriding the default positioning behavior.

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

All tests for the menu-item and menu components should pass as expected.

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

⏭ Next Steps

@radium-v radium-v self-assigned this Oct 14, 2022
@radium-v radium-v changed the base branch from master to users/jokreitl/use-floatingui-for-select October 14, 2022 23:36
Comment on lines +78 to +83
<span
?hidden="${x => !x.expanded}"
class="submenu-container"
part="submenu-container"
${ref("submenuContainer")}
>
Copy link
Member

Choose a reason for hiding this comment

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

Can we dig in here on the value of an internal element like this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the only thing we can confirm about the submenu is that it's an element with a role of menu, I think it's safer and less obtrusive to handle the positioning in the menu-item's shadow rather than on the slotted submenu element itself.

Here are some alternatives:

  • Setting the positioning styles with the style attribute on the slotted submenu can cause conflicts with specificity and author intent.
  • Attaching a stylesheet to the nested element would address the specificity problem, but it would only be possible if the submenu is guaranteed to have a FAST controller ($fastController) to attach the styles.

Copy link
Member

Choose a reason for hiding this comment

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

I agree this is probably a preferred API, though I'm certainly curious of others thoughts. Ideally we can document the intent as part of the readme, or somewhere else to help convey this. I think given the alternatives, this is the best method of providing positioning without needing to get prescriptive with whats slotted in.

@radium-v radium-v force-pushed the users/jokreitl/use-floatingui-for-select branch from b3e6aa6 to 027cb4a Compare October 24, 2022 16:53
@radium-v radium-v force-pushed the users/jokreitl/use-floatingui-for-menu-item branch from 18fd8d6 to d7904d3 Compare October 24, 2022 17:04
@radium-v radium-v force-pushed the users/jokreitl/use-floatingui-for-menu-item branch from d7904d3 to 67191c9 Compare October 25, 2022 19:01
@radium-v radium-v changed the base branch from users/jokreitl/use-floatingui-for-select to master October 25, 2022 19:01
@radium-v radium-v marked this pull request as ready for review October 25, 2022 19:02
@radium-v radium-v force-pushed the users/jokreitl/use-floatingui-for-menu-item branch from 67191c9 to 9511d06 Compare October 25, 2022 20:35
Copy link
Contributor

@EisenbergEffect EisenbergEffect left a comment

Choose a reason for hiding this comment

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

I like it.

@radium-v radium-v force-pushed the users/jokreitl/use-floatingui-for-menu-item branch from 9511d06 to 4582740 Compare October 27, 2022 18:10
@radium-v radium-v merged commit 7aaf156 into master Oct 27, 2022
@radium-v radium-v deleted the users/jokreitl/use-floatingui-for-menu-item branch October 27, 2022 20:13
@chrisdholt
Copy link
Member

Thanks @radium-v!

janechu pushed a commit that referenced this pull request Jun 10, 2024
* use floating-ui for menu-item submenus

* Change files
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.

3 participants