[SelectField] Port to next WIP#6608
Conversation
…not persisting for tab-focused SelectField'
…th form inputs, fixes dirty/clean events, and another shouldComponentUpdate
oliviertassinari
left a comment
There was a problem hiding this comment.
This is an early feedback
fixed enter and space key from opening the select
I get the same issue with th arrow keys 😬 .
| <CardHeader | ||
| avatar={<Avatar aria-label="Recipe" className={classes.avatar}>R</Avatar>} | ||
| title="Shrimp and Chorizo Paella" | ||
| subhead="September 14, 2016" |
There was a problem hiding this comment.
Already fixed on next.
There was a problem hiding this comment.
Okay, I'll merge in the latest next branch
| disabled: { | ||
| color: theme.palette.text.disabled, | ||
| cursor: 'not-allowed', | ||
| cursor: 'not-allowed !important', |
There was a problem hiding this comment.
!important looks like a hack.
There was a problem hiding this comment.
It is... I have a commit that fixes this by using a separate enabled class for the select element that sets the cursor to pointer. There might be a better way but it seems to work well.
| } | ||
| } | ||
|
|
||
| shouldComponentUpdate(nextProps, nextState, nextContext) { |
There was a problem hiding this comment.
I'm not sure about the use case for that logic.
There was a problem hiding this comment.
I'll look at this closer again. I put it in there temporarily to help track down an issue I was having with the FormControl causing an infinite loop when an onFocus handler was passed to SelectField.
There was a problem hiding this comment.
Turns out I don't need this now, which is great. I'm trying to limit my changes to just the SelectField and SelectInputField.
| {label} | ||
| </InputLabel> | ||
| )} | ||
| <Input |
There was a problem hiding this comment.
👍 For using the Input.
| @@ -0,0 +1,15 @@ | |||
| /* eslint-disable */ | |||
There was a problem hiding this comment.
We changed the way the icons are done.
Here is the check-box.js for instance:
// @flow weak
import React from 'react';
import pure from 'recompose/pure';
import SvgIcon from '../SvgIcon';
let CheckBox = (props) => (
<SvgIcon {...props}>
<path d="M19 3H5c-1.11 0-2 .9-2 2v14c0 1.1.89 2 2 2h14c1.11 0 2-.9 2-2V5c0-1.1-.89-2-2-2zm-9 14l-5-5 1.41-1.41L10 14.17l7.59-7.59L19 8l-9 9z" />
</SvgIcon>
);
CheckBox = pure(CheckBox);
CheckBox.muiName = 'SvgIcon';
export default CheckBox;There was a problem hiding this comment.
Okay, so do I just need to change /* @eslint-disable */ to // @flow weak? I can't spot any other differences.
|
Also, @jgoux has been working on an AutoComplete UI independent implementation and might have some insight on how the @jgoux Would you mind sharing your plan for the Select and how we could join forces? Thanks! |
…/down arrow key handling for opening menu
|
Hi all and thanks Olivier for mentioning my little implementation. ^^ As I see it, there is 2 distincts components to add, the In my implementation I tried to do a little of both but I don't think it's a good idea. The
The
For the async loading for both component, I'd provide an API to apply "loading" styles on these components but nothing more, I'd delegate the fetching / data providing to the user. For the support of multiple selection, I'd write a totally separated component from the For the multiple selection design, I think the Chips component (https://material.angularjs.org/latest/demo/chips) could help. Isn't a MultipleSelect a more restricted version of Chips with a contrained list of values to pick from ? I don't intend to submit a PR as the work has already begun here, but if you want to reuse part of mine don't hesitate! Also I'm sure @Sharlaan would be happy to be involved 👍 |
|
Hey @jgoux thanks for joining in, I checked out your autocomplete/select demo and it looks very nice. The select didn't seem to be working yet but the autocomplete works well. At this point, the
It sounds like the API you have in mind requires the user to pass a I just pushed a commit here with an example of a select that contains an Anyways, I agree that multiple selection component should be separate from the single select component. They are very different UX-wise, especially if we want to use chips (I think we do). Have you looked at https://github.com/JedWatson/react-select? I think we can borrow a lot of what's been done there. Maybe we can even bring it in as a dependency and style it to match? I haven't used it yet but it's very popular and already supports the features we want. |
|
Which are the differences between a Select(multiple/single), a DropdownMenu, and an AutoComplete(besides its filter) that would justify implementing 3 or more distinct components ? react-select is based on an options prop:
I think composable children would be better, especially in terms of styling where user could style his options nodes to his liking, which would require in react-select (and current Material-UI components) another separate prop dedicated to styling... Regarding integration of infinite datalist, i think it's feasible to integrate such libs with a children-based architecture. Here are 2 examples :
|
They are very similar and they can certainly share some internals, here's my take:
Those are the typical descriptions and roles of each component, they're different but are built from the same basic components:
However, I don't think we need to worry about dropdown menus, they're already implemented well. I am making sure that whatever I build is also compatible outside of the select and autocomplete components. For example, a text input specially styled for embedding in a menu can be used with any of the components.
I think internally we can reuse components like We can make reasoning about which component to use by organizing the docs well and providing lots of examples. For example, an extensive forms doc that demos the select and autocomplete in different examples.
I think this can be overcome by providing an
Yeah I've come across your other library, I'll have to look at how you integrated with |
I think this is where our opinions diverge :
yes but -again- why not with the same single all-in-one component, each of these docs example showing different props and custom renderers setups ?
Why not ? Personnally i leave this to user's imagination. ;)
Just a warning : i experienced much more difficulties to integrate with this lib than with Hope this help giving some insights ;) |
|
I've been brewing over this for a few hours and I totally agree now... If we want to allow users to compose whatever they want, then they should be able to build a select, dropdown, autocomplete, or whatever using the same components that we provide. As long as we make the most typical usecases easy to accomplish and document them we should be good. So the obvious approach would be to provide some lower level components that can be assembled to build whatever a user imagines, and then provide a few pre-assembled versions for the most common usecases:
All we need to do is continue adding Menu/List compatible components and then assemble them together for the common usecase components above. A few new components that come to mind:
So really, the big difference between select and autocomplete comes down to where the MenuList is anchored (below input for autocomplete, above the input and on the selected option for select), and which input is the "owner" (a read-only text-field for select, a text-field for autocomplete). But they share the same API for composing the menu, and for customizing how filtering works if enabled.
I haven't used either yet :) so I don't have a preference. If Thanks for the all the info! |
|
For info, i started composing with TextField + Menu and MenuItems, + react-infinite. When i wanted to manage checkIcon customisation (especially positioning), MenuItem is rigid, so i went down to ListItem, preserving focus handling. Next i wanted to propose multiple selection, but Menu seemed too rigid for this, so i went down to Popover and managed the selection logic on my own.
For the filter, i found TextField were overkill and simply used Input (since it provides the UnderLine) Next, i integrated react-infinite and lost focus handling with a filtered List. That's why i'm currently testing with react-virtualized which handles focus handling and infiniteList pretty easily. (WIP) For the main container showing selections (when menu closed, according to Specs), i were initially using TextField for its UnderLine and floatingLabel. Here again i feel this might be overkill as well, and i'm experimenting with Input or simple div container + InputLabel/UnderLine. (WIP) |
|
The implementation here is composed of I built a demo here that has a restyled I haven't messed with the multiselect yet, so I haven't ran into issues with
My vote is to provide two handlers:
The demo I built with the embedded |
|
i can help you with :
A simple idea would simply to NOT embed your filtering Input with options : <Popover> // or Menu or List whatever
<Input/> // filter
{children} // those contain user's custom options
</Popover>
Exact that's why i chose to implement my own "List" which is a simple div container exposing an onKeyDown handler managing the keypresses, and defaulting to the input inside Input (
After i switched my onChange handler to the finalized-result-delivering behavior instead of my previous deliver-on-each-selection behavior, i got a long issue discussion with some user claiming the common behavior for onChange is actually deliver-on-each-selection.
|
There is also a virtualized version : https://github.com/bvaughn/react-virtualized-select If the API is sufficient to make an adapter for Material UI I'm all for it. 👍 |
|
Brian Vaughn went even further with react-virtualized-fast-filter but i would still check how to handle styling to Material-UI, dynamic row heights and focus before validating it. |
7cf8d3c to
e4800de
Compare
|
@dummerbd Thanks for your contribution! 👍 Aside from a rebase, what's needed to move this forward? |
|
I'm closing the PR as quite old now. Let's go back to the original issue #4792. Thanks everybody for your work. Any future work will definitely have to base on what have been discussed and experimented here 💯 . |
commit 343d2f6 Author: Benjamin Dummer <[email protected]> Date: Mon Apr 17 11:11:27 2017 -0400 Added more advanced example commit 9b23596 Author: Ben Dummer <[email protected]> Date: Sun Apr 16 23:46:34 2017 -0400 Fixed cursor issue (enabled vs disabled), removed dead code, fixed up/down arrow key handling for opening menu commit c86b703 Author: Ben Dummer <[email protected]> Date: Sun Apr 16 23:44:53 2017 -0400 Removed unneeded changes to Input commit ac687d1 Author: Ben Dummer <[email protected]> Date: Sat Apr 15 06:58:53 2017 -0400 Loosens definition of controlled component so integers can be used with form inputs, fixes dirty/clean events, and another shouldComponentUpdate commit c1b99f9 Author: Ben Dummer <[email protected]> Date: Sat Apr 15 06:43:14 2017 -0400 Fixed deprecated PropTypes import, added shouldComponentUpdate to SelectField commit 5c24319 Merge: 27015ce 076adda Author: Ben Dummer <[email protected]> Date: Sat Apr 15 03:48:32 2017 -0400 Merge branch 'next' into next-select-field commit 27015ce Author: Ben Dummer <[email protected]> Date: Sat Apr 15 03:47:39 2017 -0400 Fixes tab and click focus events and focus state issues commit 48e894d Author: Benjamin Dummer <[email protected]> Date: Fri Apr 14 16:19:23 2017 -0400 Fix border radius on OSX Chrome (and maybe others) commit b6ae7e5 Author: Benjamin Dummer <[email protected]> Date: Fri Apr 14 16:08:34 2017 -0400 Fixes enter and space key from opening <select>, fixes focused style not persisting for tab-focused SelectField' commit dfc965a Author: Benjamin Dummer <[email protected]> Date: Thu Apr 13 20:18:45 2017 -0400 Fixed typo commit 0ea7142 Author: Benjamin Dummer <[email protected]> Date: Thu Apr 13 20:17:10 2017 -0400 Fixed missing import commit 2772777 Author: Benjamin Dummer <[email protected]> Date: Thu Apr 13 20:16:49 2017 -0400 Moved select field docs commit 4157310 Merge: cb89d48 9fe25c2 Author: Ruslan Kyba <[email protected]> Date: Mon Apr 10 06:12:09 2017 +0300 Merge remote-tracking branch 'refs/remotes/callemall/next' into next-select-field commit cb89d48 Merge: 4dace18 1216598 Author: Ruslan Kyba <[email protected]> Date: Mon Apr 10 06:07:25 2017 +0300 Merge remote-tracking branch 'refs/remotes/callemall/next' into next-select-field commit 4dace18 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:58:22 2017 +0200 Fixed prop description commit dd58c29 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:54:24 2017 +0200 Added ability to set custom compare function commit 33a5b3a Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:46:25 2017 +0200 Fixed value type commit 86bf0b2 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:30:29 2017 +0200 Props validation, lint fixes commit 0526359 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 08:22:43 2017 +0200 Beeter demo, proper select size commit cb094cf Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 08:04:03 2017 +0200 Different label bihaviour commit 850565b Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 05:53:47 2017 +0200 SelectField initial commit
commit 343d2f6 Author: Benjamin Dummer <[email protected]> Date: Mon Apr 17 11:11:27 2017 -0400 Added more advanced example commit 9b23596 Author: Ben Dummer <[email protected]> Date: Sun Apr 16 23:46:34 2017 -0400 Fixed cursor issue (enabled vs disabled), removed dead code, fixed up/down arrow key handling for opening menu commit c86b703 Author: Ben Dummer <[email protected]> Date: Sun Apr 16 23:44:53 2017 -0400 Removed unneeded changes to Input commit ac687d1 Author: Ben Dummer <[email protected]> Date: Sat Apr 15 06:58:53 2017 -0400 Loosens definition of controlled component so integers can be used with form inputs, fixes dirty/clean events, and another shouldComponentUpdate commit c1b99f9 Author: Ben Dummer <[email protected]> Date: Sat Apr 15 06:43:14 2017 -0400 Fixed deprecated PropTypes import, added shouldComponentUpdate to SelectField commit 5c24319 Merge: 27015ce 076adda Author: Ben Dummer <[email protected]> Date: Sat Apr 15 03:48:32 2017 -0400 Merge branch 'next' into next-select-field commit 27015ce Author: Ben Dummer <[email protected]> Date: Sat Apr 15 03:47:39 2017 -0400 Fixes tab and click focus events and focus state issues commit 48e894d Author: Benjamin Dummer <[email protected]> Date: Fri Apr 14 16:19:23 2017 -0400 Fix border radius on OSX Chrome (and maybe others) commit b6ae7e5 Author: Benjamin Dummer <[email protected]> Date: Fri Apr 14 16:08:34 2017 -0400 Fixes enter and space key from opening <select>, fixes focused style not persisting for tab-focused SelectField' commit dfc965a Author: Benjamin Dummer <[email protected]> Date: Thu Apr 13 20:18:45 2017 -0400 Fixed typo commit 0ea7142 Author: Benjamin Dummer <[email protected]> Date: Thu Apr 13 20:17:10 2017 -0400 Fixed missing import commit 2772777 Author: Benjamin Dummer <[email protected]> Date: Thu Apr 13 20:16:49 2017 -0400 Moved select field docs commit 4157310 Merge: cb89d48 9fe25c2 Author: Ruslan Kyba <[email protected]> Date: Mon Apr 10 06:12:09 2017 +0300 Merge remote-tracking branch 'refs/remotes/callemall/next' into next-select-field commit cb89d48 Merge: 4dace18 1216598 Author: Ruslan Kyba <[email protected]> Date: Mon Apr 10 06:07:25 2017 +0300 Merge remote-tracking branch 'refs/remotes/callemall/next' into next-select-field commit 4dace18 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:58:22 2017 +0200 Fixed prop description commit dd58c29 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:54:24 2017 +0200 Added ability to set custom compare function commit 33a5b3a Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:46:25 2017 +0200 Fixed value type commit 86bf0b2 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:30:29 2017 +0200 Props validation, lint fixes commit 0526359 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 08:22:43 2017 +0200 Beeter demo, proper select size commit cb094cf Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 08:04:03 2017 +0200 Different label bihaviour commit 850565b Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 05:53:47 2017 +0200 SelectField initial commit
commit 343d2f6 Author: Benjamin Dummer <[email protected]> Date: Mon Apr 17 11:11:27 2017 -0400 Added more advanced example commit 9b23596 Author: Ben Dummer <[email protected]> Date: Sun Apr 16 23:46:34 2017 -0400 Fixed cursor issue (enabled vs disabled), removed dead code, fixed up/down arrow key handling for opening menu commit c86b703 Author: Ben Dummer <[email protected]> Date: Sun Apr 16 23:44:53 2017 -0400 Removed unneeded changes to Input commit ac687d1 Author: Ben Dummer <[email protected]> Date: Sat Apr 15 06:58:53 2017 -0400 Loosens definition of controlled component so integers can be used with form inputs, fixes dirty/clean events, and another shouldComponentUpdate commit c1b99f9 Author: Ben Dummer <[email protected]> Date: Sat Apr 15 06:43:14 2017 -0400 Fixed deprecated PropTypes import, added shouldComponentUpdate to SelectField commit 5c24319 Merge: 27015ce 076adda Author: Ben Dummer <[email protected]> Date: Sat Apr 15 03:48:32 2017 -0400 Merge branch 'next' into next-select-field commit 27015ce Author: Ben Dummer <[email protected]> Date: Sat Apr 15 03:47:39 2017 -0400 Fixes tab and click focus events and focus state issues commit 48e894d Author: Benjamin Dummer <[email protected]> Date: Fri Apr 14 16:19:23 2017 -0400 Fix border radius on OSX Chrome (and maybe others) commit b6ae7e5 Author: Benjamin Dummer <[email protected]> Date: Fri Apr 14 16:08:34 2017 -0400 Fixes enter and space key from opening <select>, fixes focused style not persisting for tab-focused SelectField' commit dfc965a Author: Benjamin Dummer <[email protected]> Date: Thu Apr 13 20:18:45 2017 -0400 Fixed typo commit 0ea7142 Author: Benjamin Dummer <[email protected]> Date: Thu Apr 13 20:17:10 2017 -0400 Fixed missing import commit 2772777 Author: Benjamin Dummer <[email protected]> Date: Thu Apr 13 20:16:49 2017 -0400 Moved select field docs commit 4157310 Merge: cb89d48 9fe25c2 Author: Ruslan Kyba <[email protected]> Date: Mon Apr 10 06:12:09 2017 +0300 Merge remote-tracking branch 'refs/remotes/callemall/next' into next-select-field commit cb89d48 Merge: 4dace18 1216598 Author: Ruslan Kyba <[email protected]> Date: Mon Apr 10 06:07:25 2017 +0300 Merge remote-tracking branch 'refs/remotes/callemall/next' into next-select-field commit 4dace18 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:58:22 2017 +0200 Fixed prop description commit dd58c29 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:54:24 2017 +0200 Added ability to set custom compare function commit 33a5b3a Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:46:25 2017 +0200 Fixed value type commit 86bf0b2 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 09:30:29 2017 +0200 Props validation, lint fixes commit 0526359 Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 08:22:43 2017 +0200 Beeter demo, proper select size commit cb094cf Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 08:04:03 2017 +0200 Different label bihaviour commit 850565b Author: Ruslan Kyba <[email protected]> Date: Wed Mar 8 05:53:47 2017 +0200 SelectField initial commit
I'm not sure how to make this PR correctly since it's just my commits after @kybarg's work in #6301
My changes so far:
I'm learning a lot about MaterialUI working on this, so if no one else is, I don't mind getting this ported the rest of the way.
SelectField<select>element as base to handle autofill