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.

Sign up for GitHub

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

Jump to bottom

[SelectField] Port to v1 (WIP) #7458

Closed
wants to merge 6 commits into from
Closed

[SelectField] Port to v1 (WIP) #7458

wants to merge 6 commits into from

Conversation

ctavan
Copy link
Contributor

@ctavan ctavan commented Jul 18, 2017

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

So far this is not much more than a rebase of #6608 onto the current v1-alpha branch. Since the original PR contained merge commits I had trouble doing a proper rebase and had to squash all those commits into one.

The intention is to eventually provide a resolution for #4792, however I suppose this will need quite a bit of discussion until we get there.

In particular with respect to keyboard navigation the current state of the component is still pretty basic.

Since I'm rather new to the codebase I'd be really glad about some early feedback from somebody who's more familiar with it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 18, 2017 •
edited
Loading

@ctavan Thanks for this first contribution! I will find some time to have a look at it. Right now, I'm pretty busy upgrading the styling solution. That will eventually allow us to move to beta and I fear introduce conflict with this PR 😬 . Sorry in advance about that.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 18, 2017 •
edited
Loading

Also, I have just fixed the CI. Circle-CI changed their behavior. You can rebase if you want to run the full CI.

@oliviertassinari oliviertassinari added component: select This is the name of the generic UI component, not the React module! v1 labels Jul 18, 2017
@ctavan ctavan force-pushed the v1-select-field branch from dfbf59e to 2188ac2 Compare July 18, 2017 20:02
@ctavan
Copy link
Contributor Author

ctavan commented Jul 18, 2017

@oliviertassinari thanks for the update! Don't worry, I'm not afraid about upcoming conflicts ;)…

For now I'm mostly interested in feedback on the API of the component and what the expectations are from the Material UI library perspective.

In #6608 there was a pretty extended argument about whether Select and Autocomplete should be distinct components or whether there should be rather one beast that handles it all… Some more thoughts on these topics would be nice.

ctavan added 6 commits July 18, 2017 22:12
@ctavan
[SelectField] Squashed commit of mui#6608
2e0fbf5 
commit 343d2f6
Author: Benjamin Dummer <dummerbenjamin@gmail.com>
Date:   Mon Apr 17 11:11:27 2017 -0400

    Added more advanced example

commit 9b23596
Author: Ben Dummer <dummerbenjamin@gmail.com>
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 <dummerbenjamin@gmail.com>
Date:   Sun Apr 16 23:44:53 2017 -0400

    Removed unneeded changes to Input

commit ac687d1
Author: Ben Dummer <dummerbenjamin@gmail.com>
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 <dummerbenjamin@gmail.com>
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 <dummerbenjamin@gmail.com>
Date:   Sat Apr 15 03:48:32 2017 -0400

    Merge branch 'next' into next-select-field

commit 27015ce
Author: Ben Dummer <dummerbenjamin@gmail.com>
Date:   Sat Apr 15 03:47:39 2017 -0400

    Fixes tab and click focus events and focus state issues

commit 48e894d
Author: Benjamin Dummer <dummerbenjamin@gmail.com>
Date:   Fri Apr 14 16:19:23 2017 -0400

    Fix border radius on OSX Chrome (and maybe others)

commit b6ae7e5
Author: Benjamin Dummer <dummerbenjamin@gmail.com>
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 <dummerbenjamin@gmail.com>
Date:   Thu Apr 13 20:18:45 2017 -0400

    Fixed typo

commit 0ea7142
Author: Benjamin Dummer <dummerbenjamin@gmail.com>
Date:   Thu Apr 13 20:17:10 2017 -0400

    Fixed missing import

commit 2772777
Author: Benjamin Dummer <dummerbenjamin@gmail.com>
Date:   Thu Apr 13 20:16:49 2017 -0400

    Moved select field docs

commit 4157310
Merge: cb89d48 9fe25c2
Author: Ruslan Kyba <kybargr@gmail.com>
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 <kybargr@gmail.com>
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 <kybargr@gmail.com>
Date:   Wed Mar 8 09:58:22 2017 +0200

    Fixed prop description

commit dd58c29
Author: Ruslan Kyba <kybargr@gmail.com>
Date:   Wed Mar 8 09:54:24 2017 +0200

    Added ability to set custom compare function

commit 33a5b3a
Author: Ruslan Kyba <kybargr@gmail.com>
Date:   Wed Mar 8 09:46:25 2017 +0200

    Fixed value type

commit 86bf0b2
Author: Ruslan Kyba <kybargr@gmail.com>
Date:   Wed Mar 8 09:30:29 2017 +0200

    Props validation, lint fixes

commit 0526359
Author: Ruslan Kyba <kybargr@gmail.com>
Date:   Wed Mar 8 08:22:43 2017 +0200

    Beeter demo, proper select size

commit cb094cf
Author: Ruslan Kyba <kybargr@gmail.com>
Date:   Wed Mar 8 08:04:03 2017 +0200

    Different label bihaviour

commit 850565b
Author: Ruslan Kyba <kybargr@gmail.com>
Date:   Wed Mar 8 05:53:47 2017 +0200

    SelectField initial commit
@ctavan
[SelectField] Fix problems due to evolved upstream after rebase
3771728
@ctavan
[SelectField] Change casing of inputProps variable
f6b2e4b
@ctavan
[SelectField] Improve styling of demos
587de3b
@ctavan
[SelectField] Improve styling of SelectFieldInput
6e58e9b
@ctavan
[SelectField] Fix all linting errors and warnings
6ddcf59
@ctavan ctavan force-pushed the v1-select-field branch from 2188ac2 to 6ddcf59 Compare July 18, 2017 20:12
@charlierudolph charlierudolph mentioned this pull request Jul 21, 2017
Closed
3 tasks
@charlierudolph
Copy link

@olegberman @ctavan I would really like to have SelectField in. Anything I can help with here?

@kybarg
Copy link
Contributor

kybarg commented Jul 22, 2017

@ctavan I think that first aim is to wrap basic HTML elements into material style 😄 TextField and SelectField can both have autocompletion functionality. So I think it can be separate PR for this purpose.

@oliviertassinari oliviertassinari changed the base branch from v1-alpha to v1-beta July 23, 2017 17:09
@jgoux
Copy link
Contributor

jgoux commented Jul 24, 2017 •
edited
Loading

Hello,

I rewrote my Select component targetting the beta branch.

I rely on :

  • react-virtualized for the list performance
  • react-poppers for the list placement
  • ramda + recompose for functional utilities

Currently I only support simple select + optgroup and the implementation is pretty rigid concerning custom renderers. But it could be easily externalized as props.

Also, it would need a fallback to a native select. I think a hidden select element mirroring the selected option would be enough.

I really think a multiple implementation would require a totally different component, then the switch could be made between the simple and multiple form relying on the multiple prop.

If you want to use it as a base or for inspiration be my guess!

Here is the codesandbox : https://codesandbox.io/s/rR9BDYll4

This was referenced Jul 31, 2017
Merged
Closed
@arracso
Copy link

arracso commented Jul 31, 2017 •
edited
Loading

Hi,

@jgoux your implement of a select with search its great. As you I think that maybe the best its having to separated components if we want this behaviour.

@ctavan I took a look at the code (note that I'm still very new to js). For me it seems its going well.
On my implementation I'm just trying to do a very simple select field. As Menu component was giving me some problems (when trying to set it to same width of the input) i just decided to use a paper and implements all functionalities I need from menu my self. Now I'm current-ly trying to implement the onKeyDown behaviour of Select scrolling to options. Here you have my new code.

@kybarg
Copy link
Contributor

kybarg commented Jul 31, 2017 •
edited
Loading

@arracso @ctavan @jgoux I think it would be nice to take Angular Material as example.
I think also that autocompelte should be based on TextField component rather then Select component.
And select should have a search box like this https://material.angularjs.org/latest/demo/select#select-header

@rosskevin
Copy link
Member

@kybarg, mentioned in #7477, AutoComplete is left as an integration and is entirely isolated from Select.

@arracso
Copy link

arracso commented Jul 31, 2017

Just added scrolling to my SelectField, (it scrolls to the first element starting with key pressed).
I wont touch it anymore (have work to do).

@kybarg kybarg mentioned this pull request Aug 2, 2017
Closed
@rosskevin
Copy link
Member

Let's move forward with #7632

@rosskevin rosskevin closed this Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
component: select This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ctavan @oliviertassinari @charlierudolph @kybarg @jgoux @arracso @rosskevin