Feature/migrate enhanced choice tui

[kinishinai12]

Hi Kuya @nicolas-jaussaud and Ate @juliacanzani, I’ve opened a draft PR for the Enhanced Choice migration to Tangible UI.

I’ve removed the react-aria and react-stately dependencies and migrated the component to use Tangible UI components where applicable.

When you have a chance, could you please review it and let me know if there are any other parts of the component that you think could be further migrated or improved to better align with the TUI approach?

Thank you!

[juliacanzani]

Hey @kinishinai12! Thanks for the work on this! There are a few accessibility and layout issues that will need to be resolved before this can be merged.

First, the layout shifts when the field moves between states due to the chip in the top right corner being replaced with linked text. I'd suggest using a button component here and disabling it in non-interactive states. The "ghost" variant will be good for the confirm selected state!

Second, I get the sense that you haven't tried interacting with your field much using only your keyboard! It's important that the user can access the options in the dropdown menu without using a mouse (navigating with tab/shift-tab/arrow keys/space/enter), and as far as I can tell this isn't working properly on the first activation after loading the page, and doesn't seem to play nice with the mode that supports custom values in my testing.

Grouped items mode is a nice addition, but I think it might look better if you could put the "Select all" checkbox inline with the section heading.

There are some questionable style choices here, particularly the pink borders in the review choices list. It would be nice if we could try to match the figma a little more closely in terms of item treatment, and maybe consume TUI primitives so that when somebody adjusts colors, border-radiuses, etc. your component isn't left behind :)

I'm also a bit concerned about the choice to make the view link a prop instead of maybe doing something more composable to allow the item to be populated with whatever content is contextually relevant (TUI's dropdown is a good example of this pattern)

[kinishinai12]

Hello ate @juliacanzani and Kuya @nicolas-jaussaud, I've made the changes and added the recommendations ate Julia suggested, especially around making the item composable, I configured it as a prefix/suffix slot system rather than literal children based, since this component's items can come from php as data.
So instead, PHP configures each slot by naming a registered component: icon, badge, viewLink, and button and its props from tangible UI. Let me know what you think.. Thank you!

[nicolas-jaussaud]

Hi @kinishinai12!

I left a few comments regarding things I think we should change before merging

It's mainly some standardization to be in line with existing syntax/feature, and to avoid duplicating existing behavior

If you have the time, I'd also suggest adding some simple jest tests for every configuration to avoid future regression

Other than that it looks great to me, the control is going to be super useful and will support a lot of cases!

@juliacanzani should probably have a second look too, as she will be more able to evaluate if tui is integrated as it should and make sure there are no ux/accessibility issues

[nicolas-jaussaud]

This file and the associated map file should probably be removed, I don't think they are currently used (it's probably the same thing for the new index.min.module.css file + the associated .map)

[nicolas-jaussaud]

We use a very similar syntax/feature for ComboBox and Select fields with categories (see here and here):

[
  {
    name    : 'Category 1',
    choices : {
      value1 : 'Value 1',
      value2 : 'Value 2',
    }
  },
  {
    name    : 'Category 2',
    choices : {
      value3 : 'Value 3'
    }
  }
]

If possible, we should try to re-use the same names for consistency

I'm also wondering if we could simplify and remove the isGrouped prop, and determine which layout should be used based on the shape of the data

For example, if an item contains choices or items, we know we are inside a grouped list (which would also be in line with what we do for other list based component)

[nicolas-jaussaud]

While we can't use the legacy <Label /> and <Description /> components as they rely on react-aria, tangible-ui has some replacement (there are some example on tui storybook here)

We should probably use them here as well (for label, description, container... etc) as it will make sure we get the same behavior/style across all fields

Another nice bonus is that it will add supports for two props we miss currently, labelVisuallyHidden and descriptionVisuallyHidden:

<Field // ... >
  { props.label &&
    <Field.Label
      hidden={ Boolean( props.labelVisuallyHidden ) }
    >
      { props.label }
    </Field.Label> }
  <Field.Control>
    // ... 
  <Field.Control>
  { props.description &&
    <Field.HelperText
      className={ props.descriptionVisuallyHidden ? 'tui-visually-hidden' : undefined }
    >
      { props.description }
    </Field.HelperText> }
</Field>

It's important we support them, and some jest tests will fail if we do not

There are some examples of implementation in new fields Julia worked on:

• the new <Text /> component (here)
• the new DatePicker (here)

[nicolas-jaussaud]

We should add every new attributes here as well (is_grouped, is_viewable, is_custom_mode_enabled, item_layout )

I imagine we should update the associated test case too (this one)

(Sorry it's a bit annoying to have to maintain this list by hand, I should probably change how it works and automatically format everything from snake_case to camelCase at some point)

[nicolas-jaussaud]

We should remove the changes from this file

We updated to some packages that now require PHP 8.4. While those packages are dev only, it will still causes issues in our test workflow, as we install composer dependencies for PHP 8.2 (the associated workflow error can be seen here)

[nicolas-jaussaud]

Good idea to add a way to render component from the php side!

One thing we could do to avoid having to maintain a list of accepted componentq is to use the registered elements (from here) and add what's missing there (like the Icon component)

I don't think that would change the logic much, and we would benefit from every future element we register out of the box

An element registered in this list can then be rendered from the PHP side, like we render a field:

$fields->render_element( 'icon', [ 'type' => 'icon', //...config ] );

So we could use a similar syntax for prefix/suffix:

'prefix' => [
  [
    'type'   => 'icon',
    'name'   => 'edit'
  ]
];

From the JS side, any element can be easily rendered (using this component):

<Element type='icon' name='edit' />

Just an idea, but I'm also wondering if we should consider setting the prefix/suffix on a choice level to allow completely different configuration per row, like this:

'choices' => [
  'value1'  => [
    'label'  => 'Value 1',
    'suffix' => [
      [
        'type' => 'badge',
        'children' => 'Popular'
      ]
    ],
  ],
  'value2' => [
    'label' => 'Value 2''
  ],
],

We could keep the prefix/suffix definition inside itemLayout, and add the possibility to overwrite it like this

Lastly, we could also consider allowing react components to be passed directly as an argument, when we are rendering from the JS

Feel free to ignore some/all of this feedback, it's just some idea I wanted to share