cerner / terra-core

Terra offers a set of configurable React components designed to help build scalable and modular application UIs. This UI library was created to solve real-world issues in projects we work on day to day.
http://terra-ui.com
Apache License 2.0
183 stars 165 forks source link

[RFC] Intent to split terra-select into multiple packages #2492

Closed bjankord closed 5 years ago

bjankord commented 5 years ago

Feature Request

Description

We've been discussing the idea of splitting the terra-select into multiple packages to make it easier to maintain and test as features and bugs are worked on within the component.

Currently, there is a high amount of code reuse within the package which is good, however there is also high level of understanding one has to maintain as they work on the package to ensure there are not a regressions in one of the 5 variants in one of the many browsers, screen readers, and devices we support.

To help lower this level of understanding, we are looking to break the terra-select package into multiple packages.

Phase 1

The first part of this work would be to split the existing variants in terra-select into multiple packages as-is with the current form of the code moving from all being in 1 package to being in the multiple packages.

Phase 2

Uplift code to resolve accessibility concerns, async concerns, and any other issues we want to address as we work through this uplift.

Work needed for phase 1

For phase 1, we need to reach consensus on how we want to split apart the terra-select package. Based on discussions we've been having, we have the following break down.

Current Variants Phase 1 Update Phase 2 Update
Default Stays in terra-select, no plans to add async option No change
Combobox Moves to terra-combobox. Fix accessibility issue with VoiceOver on iOS. Add ansync option.
Search TBD Fix accessibility issue with VoiceOver on iOS. Add ansync option.
Multiple Moves to terra-multi-select. Fix accessibility issue with VoiceOver on iOS. Add async option.
Tag Moves to terra-multi-select. Fix accessibility issue with VoiceOver on iOS. Add async option.

Some questions with this:

Some details of Phase 2 may be flushed out more as we get closer to that part of the update, however, it would be good to discuss parts of phase 2 as we work on phase 1.

bjankord commented 5 years ago

Potential diff for multi-select.

import React from 'react';
- import Select from 'terra-form-select';
+ import MultiSelect, { Option } from 'terra-form-multi-select';

const MultipleExample = () => (
-  <Select placeholder="Select a color" variant="multiple">
-    <Select.Option value="blue" display="Blue" />
-    <Select.Option value="green" display="Green" />
-    <Select.Option value="purple" display="Purple" />
-    <Select.Option value="red" display="Red" />
-    <Select.Option value="violet" display="Violet" />
-  </Select>
+  <MultiSelect placeholder="Select a color">
+    <Option value="blue" display="Blue" />
+    <Option value="green" display="Green" />
+    <Option value="purple" display="Purple" />
+    <Option value="red" display="Red" />
+    <Option value="violet" display="Violet" />
+  </MultiSelect>
);
cody-dot-js commented 5 years ago

So, I think it may help to define the intent of these components.

Components

Select

A control element for displaying a list of single-selectable options.

Combobox

An "editable" single select. You can add new options to the select using an input. Will also filter down the displayed options list based upon the input's value.

Search

A "non-editable" single select with the ability to search (or filter) with an input for a specific, single option. Useful for finding a specific option in a large list of options. Similar to a combobox, but does not allow you to add new items.

Multiple

A "non-editable" multi select. Allows for one or more selections. Also has the ability to search/filter down the list using an input.

Tag

An "editable" multi select. Allows for one or more selections. You can add new options to the select using an input. Will also filter down the displayed options list based upon the input's value.

Async Types

For async variants, I really like react-select's API.

Other notes

Select

Doesn't make sense to have async. The list for a single select should not be large enough to warrant async fetching. If async loading is needed, it should be a userspace concern, e.g. async load the data and then display the select with those options only when the data returns.

Editables (Combobox, Tag)

Add props for onAddOption / onRemoveOption?

cody-dot-js commented 5 years ago

Does it make sense to keep the children API, even for the non-async variants? It's not like you could use anything other than <Option /> for each child option. The only benefits I see here is that you allow the user to define their own keys directly, it more closely matches the API of a native <select>, and would be easier to split initially since it wouldn't require code changes.

If we switched to a data prop API instead, they would just need to feed a list of options to the component, (which could include a key, else fallback to the value) e.g.:

const data = [{
  value: 'one_fish',
  display: 'Two Fish',
  key: 'one_fish',
}, {
  value: 'two_fish',
  display: 'Two Fish',
}, {
   value: 'red_fish',
  display: 'Red Fish',
}, {
  value: 'blue_fish',
  display: 'Blue Fish',
  key: 'dr-seuss'
}];

<Select data={data} />

In this scheme, we would just need to maintain (a list of) selections of the same shape, { value: String, display: String } (would be dope if we could support React nodes here too, like for <FormattedMessage />). So now, the user can make their select async by nature of the API: just pass in new data. Thoughts?

StephenEsser commented 5 years ago

React is designed to be a semantic compliment of HTML. I am in favor of preserving the children based API, especially for the default Select variant. It is much smoother and semantically identical to the expected markup of the component and doesn't require defining a constant outside of the render return or creating a blocky object based inline data structure.

For Async functionality the data API may more user friendly and nicer to tie into. It's possible to design an API that allows both. Ant Design does something similar with their Tree and Select Tree. treeData and children are both accepted ways to populate the tree.

https://ant.design/components/tree-select/

https://ant.design/components/tree/

cody-dot-js commented 5 years ago

I think that's okay. However, as far as making the component accept multiple APIs, eg. children & data, I think we should err on the side of simplicity. As they say,

less code === less bugs

One of the reasons why we're proposing this intent to split up terra-form-select in the first place is because the code has become a monorepo itself. It's so complex and abuses DRY to the point of being an unmaintainable monster.

That being said, I understand and appreciate the design goals of React wanting to maintain a semantic complement of HTML, but it still seems a bit odd to have to do something like:

import React from 'react';
import Select, { Option } from 'terra-form-select';

function FooSelect({ options }) {
  return (
    <Select>
      {options.map(option => <Option key={option.value} value={option.value} display={option.display} />}
    </Select>
  );
}
  1. It's almost never the case that we have a static list of <Option> defined within a component. Generally, a list of data/options comes in as a prop and we have to map over them anyway.
  2. We're already breaking from a typical HTML flow by using the display prop on the <Option> instead of passing the display as a child
  3. How much value do we gain, as consumers, from having to know that you need to import { Option } from 'terra-form-select' (or use <Select.Option> in the current case) and pass a key to each option (yes, this is fundamental React knowledge, but as I said above, less code === best code)? I'd argue very little since:
    1. I'm not aware of a case where OCS allows the ability to define custom options
    2. This is essentially boilerplate code that can be removed from the consumer without seeming that magical.
    3. This (the data api) is already a common pattern in other React based select libraries.
  4. as an addendum to #3, the best thing about the children api is that it easily allows for use of <OptGroup>s, however the same could be applied to a data api (not that this is an ideal api for that)

One thing that I think could be simpler to do with the list of options (while also still possible with the children api) is windowing. Windowing would vastly improve perf for large lists. That being said, it doesn't make sense to use a default select for cases involving large lists, hence where the searchable variants come into play.

Now, the async versions should alleviate most of the perf issues we see with large lists, since they should hit a backend which filters down the returned list based upon the given input, but I think we should still explore windowing. While we are presumably being returned filtered data, it's not unreasonable to assume that we would still see large lists. According to https://github.com/cerner/terra-core/issues/2161, we see that lists with > 100 items can already start to impact perf.

image

I went ahead and did some perf tests myself. This was in production mode with the current terra-form-select: https://github.com/dev-cprice/terra-select-perf

Results found: here

While the numbers are smaller than the table above, this is run on a pretty powerful laptop. Obviously, the numbers will be much worse on lower-powered devices, e.g. mobiles.

Coming back to a previous point about <OptGroup>s, async versions are presumably going to want to support the same feature. If this is the case, it may make sense to explore a good interface for a data API here. Again, the native mobile space has tackled this problem before, so maybe we can look to them or terra-list with sections for inspiration (essentially arrays of arrays).

cody-dot-js commented 5 years ago

As far as the a11y changes needed for the searchable variants, @bjankord and I talked with multiple UX designers and iterated on changes until we landed on a solution that has been brought up before: put the input in the select's dropdown menu for iOS devices. This fixes the problem of the virtual indicator not following the DOM as it appears since the input is now a direct sibling (as opposed to a visual sibling) of the list of select options.

There is a tech design for making a responsive dropdown https://github.com/cerner/terra-core/issues/2405, but this involves a lot (read as: extreme amount) of extra complexity and extra dependencies. To name a few:

This arguably introduces way too much complexity when we're already breaking this component up. This has also seen kickback from consuming teams because the UX is strange. It's also ambiguous by the current zeplin designs of what every state entails. Does a selection in the modal only get committed when the user taps the apply button? Or is the apply button simply a glorified close button, mirroring the top right close button? Is the input sticky with the header? Does it really keep growing with each new selection from the added tags? Etc, etc (lots of unknowns, yikes)


So, with the simplest approach of portalling the input as well, we get the following:

On desktop devices, the workflow is unchanged.

On iOS (and mobile in general), the workflow is essentially:


Edit:

UX has stated that the intended design of the terra-form-select should not be deviated from, hence why we need to place the tags in the dropdown menu's input as well (replicating them). The goal is to appear like we aren't portalled because this should be an implementation detail that doesn't matter to the end user. This means that the portalled dropdown menu should appear identical to this:

image

cody-dot-js commented 5 years ago

Added a new RFC https://github.com/cerner/terra-core/issues/2497 to explore switching to the data API.

bjankord commented 5 years ago

I'd like to get the following RFC squared away and resolved before we resolve this RFC as it will base some of the direction we need to take for #2492

tbiethman commented 5 years ago

After more discussion, we've landed on continuing to keep 1 package, terra-form-select. Instead of splitting this package into multiple packages, we've discussed splitting the rendering of each variant into its own file and having Select.jsx act as an abstraction layer of the different renderings using the variant prop to drive a switch statement for selecting which rendering to use.

There is some logic for this already in Select.jsx

We'll be separating the rendering even more so than it currently is.

The pros of this include:

cody-dot-js commented 5 years ago

Made a repo to create a minimal example of changes necessary to add async data to terra-form-select here: terra-select-async. The initial commit is simply a fork of the current terra-form-select v5. I've made a PR here which should make it easy to diff between changes to the fork.

Plan (WIP)

bjankord commented 5 years ago

@dev-cprice Let's move the last comment you had on this issue over to https://github.com/cerner/terra-core/issues/2497