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: add FloatingMenu to dropdown #7928

Conversation

ConradSchmidt
Copy link
Contributor

@ConradSchmidt ConradSchmidt commented Mar 1, 2021

Closes #7565

Adds a FloatingMenu to the dropdown component, to use it within scrollable areas

Changelog

New

Changed

  • Dropdown with FloatingMenu

Removed

Testing / Reviewing

  • open dropdown story "Overflow" and "Overflow in Modal" to check if the dropdown menu is shown properly (not cut off by the overflow area)

Open points

  1. Should this be a default or triggered by a property?

  2. Should this be added to multi select?

  3. This is required by the FloatingMenu, should these properties optional? (Code)

          menuRef={() => {}}
          menuOffset={() => {}}
  1. This is needed for the modal to work (otherwise it closes on select) - any other ideas to fix this? (Code)
          <ListBox.Menu
            onMouseDown={e => e.stopPropagation()}
  1. The FloatingMenu works fine except that it is not taking scrolling into account and should be addressed in another issue
    carbon-dropdown-scroll-issue
    A simple solution for the dropdown would be to close it on scroll, to avoid updating the menu via JS (because this is laggy). For other components like the Tooltip this could work as well, but I guess this needs to be discussed in the separate issue.

@ConradSchmidt ConradSchmidt requested a review from a team as a code owner March 1, 2021 16:34
@netlify
Copy link

netlify bot commented Mar 1, 2021

Deploy preview for carbon-elements ready!

Built with commit 6ddd093

https://deploy-preview-7928--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Mar 1, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit b46f1b3

https://deploy-preview-7928--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Mar 1, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 6ddd093

https://deploy-preview-7928--carbon-components-react.netlify.app

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

thanks for jumping in on this issue, the menu placement looks good so far. currently keyboard navigation is not supported in the floating menu though, which is a regression

to answer some of your questions:

  • Should this be a default or triggered by a property?

I'm leaning towards this being triggered by a property because of the scrolling issue you mention below

  • Should this be added to multi select?

yes I believe eventually this could make its way to the other listbox components but we can focus on dropdown for the time being

  • This is required by the FloatingMenu, should these properties optional? (Code)
          menuRef={() => {}}
          menuOffset={() => {}}

I believe menuRef will be required to find the menu body and focus on the proper element, but menuOffset probably won't be needed

  • This is needed for the modal to work (otherwise it closes on select) - any other ideas to fix this? (Code)
          <ListBox.Menu
            onMouseDown={e => e.stopPropagation()}

I don't think there's a way around this, since the floating menu is no longer a child of the modal, it views clicking inside the menu as clicking outside of the modal, which closes the modal by default. Probably another reason to go with prop vs default behavior (referring to question 1)

@ConradSchmidt
Copy link
Contributor Author

ConradSchmidt commented Mar 5, 2021

  1. I added a detachMenu property, but I'm not happy with this name. appendTo would be better, but that would imply to provide an elementRef, but I didn't got this to work
  2. I ported the fix to the multiselect dropdown, but it is not working (visually) 100% - I guess its easy to fix, but first I want to have an approval for the general fix, otherwise I would have to rework the whole multiselect
  3. menuRef is just a callback - there is some logic going on which triggers the proper placing of the menu - somehow providing a function works here. maybe @asudoh can help out here?
  4. okay 👍
  5. ...nothing new...

the menu placement looks good so far. currently keyboard navigation is not supported in the floating menu though, which is a regression

  1. The problem was that the detached menu wasn't focused and didn't received any keyboard inputs. I fixed it by just attaching the onKeyDown event on the button itself (most easy solution). I see 4 other solutions here:
    6.1 forward all events to menu (you can “retrigger” events on the menu - but quite “complex”)
    6.2 render an attached “hidden” menu to still trigger the events
    6.3 somehow focus the detached menu (didn’t get it to work)
    I also had to add a ForwardRef to the FloatingMenu, otherwise the onSelect event wasn't fired

Working keyboard navigation:
carbon-dropdown-keynav-issue

Base automatically changed from master to main March 8, 2021 16:35
@ConradSchmidt
Copy link
Contributor Author

  1. Updated the solution to use a wrapper element to get the activeElement state on the detached menu (solution 6.3)

Also:

  1. Solved the styling issue for the MultiSelect
  2. Removed the ForwardRef from FloatingMenu

@ConradSchmidt
Copy link
Contributor Author

@emyarod are there any updates on this PR? Is this solution still viable or are there any other solutions to this issue?

@emyarod
Copy link
Member

emyarod commented Mar 29, 2021

there are still some open questions from our last conversation, mainly looking for any downshift examples where a floating menu is used. I still have concerns about this use case and its compatibility with downshift

@tay1orjones
Copy link
Member

I'm going to close this due inactivity, but feel free to reopen if the use case concerns and downshift compatibility can be addressed

@github-actions
Copy link
Contributor

DCO Assistant Lite bot: Thanks for your submission! We ask that you sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text:


I have read the DCO document and I hereby sign the DCO.


Conrad Schmidt seems not to be a GitHub user. You need a GitHub account to be able to sign the DCO. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

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.

Dropdown menus should overlay scrollable areas
3 participants