[SelectField,TextField] Refactoring/Popover/Keyboard

[chrismcv]

This PR does several things...

1. <SelectField /> no longer uses dropdown menu
2. Introduces <TextFieldDecorator /> which is used to wrap an input element e.g. TextInput or SelectInput with floatingLabel, errorText, "animated underline of focus". (This one is quite a useful change moving forward. I'm not sure about the naming (as decorator has ES7 implications), so any thoughts welcome...)
   • I think this should lead to <TextField /> and <TextFieldMultiline /> in due course.
     3, Adds keyboard support to SelectField ([SelectField] Keyboard navigation support #3583)
3. Adds on mount open support toSelectField superceding https://github.com/callemall/material-ui/pull/3403/files
4. Removes "tab" support for toggling through menu-items because this seemed silly.
5. Has a number of "magic numbers" to keep pixel precision with the current material-ui select-field and text-field demo pages... I think there is room to get rid of these, but that can be a next PR.

---

• This PR has no tests. It doesn't change behaviour, but what it is changing is covered by no tests.
• 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).

[chrismcv]

@mbrookes - as discussed - sorry for the delay... few issues...

[chrismcv]

@alitaheri - you might be interested in SelectFieldLabel - I had to make it a React Component vs a stateless functional component so that I could ref and focus it... I don't quite understand the react-warpgate readme, so not sure if it would help in this situation, but I suspect it might just be able to?

[nathanmarks]

Small note: use rule specific comments for disabling, eg:

/* eslint react/no-did-mount-set-state: 0 */

Also, why do we need to use setState() in componentDidMount?

[chrismcv]

I think it's needed, because we need to set an anchorElement for the popover. If you know a better way, I'd be happy to drop in!

[nathanmarks]

I have a couple of other things related to the state I want to discuss, but just as a quickie: will it not work properly in componentWillMount?

[chrismcv]

it does work - didn't think it did...

[nathanmarks]

@chrismcv Can we split out the smaller, unrelated tweaks into another PR?

Also, I just want to make a note RE tests since you highlighted it: I'm not sure if the fact that tests don't already exist is a good reason not to add any. In fact, with a refactor to existing components, I'd argue that:

1. Testing is even more important to ensure we don't break existing behaviour.
2. It is the ideal time to add tests as you are knee deep in it.

[chrismcv]

to confirm, by smaller do you mean changes to a) menu-item, b) date-picker-inline...? if so, yes... I think the rest is best dealt with as a whole.

On the testing note... I agree, but as my original goal was adding keyboard support, introducing a full unit test suite for text field and select field seemed like a bit of a distraction.

[nathanmarks]

@chrismcv Yep, I think the TextField and SelectField changes might result in a bit of discussion -- so it makes sense to get the others merged independently.

[nathanmarks]

@chrismcv Hmm. I've just realised that there is a problem with having open as a prop with the way SelectFieldis currently designed.

@newoga I'm having 2nd thoughts about that suggestion I made RE open. (in another post, a few days back)

Here's the problem: we have 2 sources of truth for open since props.open can be true, while internally, the component closes itself by using setState. Unless I'm interpreting the code incorrectly (I haven't checked out the PR yet), this means that the menu will close, but props.open will still be true. SSCREEEEEECH. The next time the parent re-renders or the props change, the select will open again by itself 😟 With that considered... this setup works fine for the previously suggested behaviour openOnMount (as long as it's only in componentWillMount, not componentWillReceiveProps), but not for open.

(I still need to verify this, so correct me if I'm wrong!)

edit removed something about open vs openOnMount that I hadn't wrapped my thoughts up on properly

[nathanmarks]

@newoga Completely separate from my other feedback... Remember that one of the reasons we want to refactor TextField is so that we can improve performance. Is this a step in the right direction for that? Just want to make sure that if we're doing some heavy refactoring -- that it will play nice with other refactoring plans we have 👍

[nathanmarks]

@chrismcv Sorry for the intense reviewing 😆 the TextField is a key component and we've had a lot of discussions about its problems lately.

[nathanmarks]

@chrismcv if you could summarize in one sentence: what's the main goal for the refactoring here?

[newoga]

Yeah I think we have some design considerations we need to consider (for all of these components, but especially as it relates to TextField).

@chrismcv if you could summarize in one sentence: what's the main goal for the refactoring here?
Show all checks

I'd be curious to hear this too. 😄 As @nathanmarks mentioned, there may be an opportunity to split this up, or to consider these changes holistically with some of the other design and performance considerations we've had recently.

[alitaheri]

any chance we can use flex box? 😁

[chrismcv]

The purpose of refactoring <TextField /> is to allow the focus to be controlled declaratively from the SelectField - and adding in further complexity to <TextField /> as is seemed like a bad idea.

The purpose of refactoring <SelectField /> is to reduce the deep nesting of components and remove hacks to make <DropDownMenu /> appear inside a <TextField /> nicely.

[nathanmarks]

@chrismcv Thanks, that's very helpful!

I want to chat with @newoga some more about the pros/cons of this refactor and how it fits into fixes for the other issues that TextField has. One of the reasons we're hesitant, is not necessarily because we think this is a bad idea, but because we were considering making breaking changes to TextField in the 0.16.0 release. As I'm sure you can imagine, not having the shackles of breaking-changes allows for "free-er" refactoring -- we saw addressing the performance issues (amongst other things) as one of the key parts of the breaking refactor, something that this PR doesn't address and we want to make sure that the work here, if merged, doesn't go to waste and end up getting refactored again.

This would be my recommendation for next steps:

1. Split out the small changes so we can merge them
2. Put this PR on hold until 0.15.0 is released. I don't think this refactor is necessary for 0.15.0 and it's a big shuffle to make with no supporting tests so close to a new release. @newoga and I have put several other TextField related PRs on hold and paused some other discussions until after this (overdue) release. (Char count, slow performance, etc)
3. After 0.15.0 is released, we can regroup and look at this PR then to assess if these changes are a good way to move forwards and perform additional refactoring on top of (performance issues, code optimizations, more composability, etc).

Thanks so much for the hard work, we really appreciate it. Does that plan sound good to you?

[nathanmarks]

@callemall/material-ui any opinions on this type of design if we refactor this for 0.16.0?

[nathanmarks]

@chrismcv are you able to rebase this?

[chrismcv]

@nathanmarks - to be honest, the amount of change in this vs the amount of change in the structure and layout of the project all likely means that redoing it from master will be cleaner (and less likely to break something subtle.) Happy to redo for 0.16.0 if you guys are happy with the approach - if you can confirm that first, and I'll endeavour to start some evening this week?

[nathanmarks]

@chrismcv let's get some more opinions -- I also need to revisit it myself too

[AlastairTaft]

#714

Can't test this branch due to not being able to build it with #4337. But by reading the comments, it looks like it will solve non tab-able dropdowns by making them tab-able. Is this something that might get merged into the next version? Would love to see tab-able dropdowns in an upcoming release

[nathanmarks]

@chrismcv closing this for now since you'd rather reimplement it anyways -- but let's revisit after style changes.