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

Add floating ui to Sort section and filterByOpts in MultiAddSelect component #6328

Open
Tracked by #6121
devadula-nandan opened this issue Oct 30, 2024 · 4 comments · May be fixed by #6389
Open
Tracked by #6121

Add floating ui to Sort section and filterByOpts in MultiAddSelect component #6328

devadula-nandan opened this issue Oct 30, 2024 · 4 comments · May be fixed by #6389
Labels
component: AddSelect needs: design opinion Design question needs opinion from designer priority: medium Pick up in the next two quarters role: dev status: blocked 🧱 type: enhancement 💡 New feature or request
Milestone

Comments

@devadula-nandan
Copy link
Contributor

devadula-nandan commented Oct 30, 2024

the requirement for this issue is to migrate overflowMenu usage from @carbon/react in our MultiAddSelect component from v11 to v12 via feature flag enableV12Overflowmenu.

current v11 usage screenshots

Image 1 Image 2

Why is this necessary?

To stay ahead as early adopters, identify potential conflicts with the upcoming major version, and ensure we receive the latest and ongoing support for both Floating UI and the new version.

Why is this blocking us?

The technical challenge lies in the shared MenuItem component between the Menu and the v12 OverflowMenu. The behavior of this shared child differs when used with the OverflowMenu parent, specifically preventing the renderIcon prop from working unless the parent is set to mode="basic", a configuration only supported by the Menu component.

Additionally, the label prop and shortcut prop for MenuItem in v12's OverflowMenu is restricted to a string type, which limits our ability to render icons alongside the label. In contrast, the previous v11 OverflowMenu used its own child component, OverflowMenuItem, which allowed more flexibility by supporting a React node for the itemText prop. This enabled us to render custom components, such as icons, within the menu.

this is a hard block unless the v12 overflowMenu provides a way to get renderIcon to work, and checkbox on menuItem to work

@github-project-automation github-project-automation bot moved this to Needs triage 🧐 in Carbon for IBM Products Oct 30, 2024
@elycheea elycheea moved this from Needs triage 🧐 to Backlog 🌋 in Carbon for IBM Products Oct 30, 2024
@devadula-nandan devadula-nandan moved this from Backlog 🌋 to Needs review 👋 in Carbon for IBM Products Nov 7, 2024
@devadula-nandan devadula-nandan moved this from Needs review 👋 to In progress in Carbon for IBM Products Nov 8, 2024
@devadula-nandan devadula-nandan removed the status: needs design review 🎨 Component is ready for design review label Nov 25, 2024
@devadula-nandan devadula-nandan moved this from In progress to Backlog 🌋 in Carbon for IBM Products Nov 25, 2024
@devadula-nandan devadula-nandan added the needs: design opinion Design question needs opinion from designer label Nov 25, 2024
@oliviaflory
Copy link

oliviaflory commented Nov 25, 2024

Adding some notes to this issue, cc @devadula-nandan @elycheea @RichKummer

Overflow vs Popover

Carbon is suggesting we use a popover for the filter with checkboxes

The static icon part of this issue is resolved by #18153. For the the interactive checkbox designs we actually recommend using a popover where the menu requires non-menu actions like shown in the filter. Here is an example in this data table story of the suggested solution for filter menus.

Design considerations:

Image

Arrows are not clear

The up and down arrows are not quite clear as to what is happening when the user selects one of the items in the menu. The designers on the standup call agreed that text written out as ' Title ascending' and `Title descending' would be more easily understood than the arrow icons and could be a better design

Other design options:

  1. File sorting can be done with only ascending
    Image

  2. An alternative icon would possibly make the filters more clear - we have a similar icon in our icon library but I might argue it's not as good as the example shown because the arrows are both pointing down and you have to look at the lines for indication of order

Image
Image

  1. Are ascending and descending items just another way to filter? Should they be included in the filter icon and not a separate icon?
    Image

@ljcarot ljcarot removed status: blocked 🧱 needs: design opinion Design question needs opinion from designer labels Jan 6, 2025
@ljcarot ljcarot moved this from Backlog 🌋 to Needs refinement 🤓 in Carbon for IBM Products Jan 6, 2025
@ljcarot ljcarot added component: AddSelect type: enhancement 💡 New feature or request priority: medium Pick up in the next two quarters labels Jan 6, 2025
@ljcarot ljcarot added this to the 2025 Q1 milestone Jan 6, 2025
@matthewgallo
Copy link
Member

Currently blocked by carbon-design-system/carbon#18153, when @janhassel's PR is merged we'll be able to support the checkbox within MenuItems.

@matthewgallo
Copy link
Member

  • MenuItem with icons, will be changing text only
  • Potential option of changing MenuItem with checkboxes to MenuItemSelectable
    For example:
<Menu open>
  <MenuItemGroup label="Font style">
    <MenuItemSelectable label="Bold" selected />
    <MenuItemSelectable label="Italic" />
  </MenuItemGroup>
</Menu>

Otherwise we keep MenuItem with checkboxes inside of a popover.

  • Changes to design kit and website if we use MenuItemSelectable

@matthewgallo matthewgallo added the needs: design opinion Design question needs opinion from designer label Jan 8, 2025
@elycheea
Copy link
Contributor

elycheea commented Jan 8, 2025

More detail on checkboxes in the menu/flyout —

The original Add and Select pattern was designed in v1 (Carbon 10), where the Menu and Popover were not available, which meant that we resorted to using the OverflowMenu

We’re currently seeing the following options —

  1. If we still consider this this as a menu, then we can probably use MenuItemSelectable as @oliviaflory previously demonstrated —

Image

In our case, it actually may be MenuItemSelectable for the types/checkboxes (pdf, jpg, xml) that can be selected independently and MenuItemRadioGroup for sorting where only one option would be active at a time. If we decide to move this direction, we do need to decide whether to consolidate these into a single filter menu.

This option would also require more design changes in both the kit and website docs, but @davidmenendez does not believe it would result in any breaking changes based on the current implementation.

  1. This also works very similarly to other filter flyouts that we have, in which case, we could also stick with the current implementation using the multiple menus. In this case, we would move to the checkboxes to use Popover as suggested by Carbon (see Olivia’s comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: AddSelect needs: design opinion Design question needs opinion from designer priority: medium Pick up in the next two quarters role: dev status: blocked 🧱 type: enhancement 💡 New feature or request
Projects
Status: Needs refinement 🤓
Development

Successfully merging a pull request may close this issue.

5 participants