Refactor switches, simplify code, fix related bugs. #5780

nathanmarks · 2016-12-13T23:27:02Z

[wip] Still a couple of things to solidify 100% (and get the keyboard focus styles working for the switches)

Refactors SwitchBase into a factory
Refactors SwitchLabel into HoC which acts as a wrapper
Refactors Checkbox, Radio, Switch to use new Factory
Drastically simplify RadioGroup, now it just provides the onChange/selectedValue helpers.
Fixes keyboard functionality for groups of radios and you no longer need the RadioGroup for proper keyboard functionality as the native elements drive it using the name="".
Removes the need to add a lot of manual markup for AT (aria attrs etc)
REMOVE CODE :)

The changes here allow us to simplify the code significantly with regards to everything from accessibility, complexity related to implementing native-like keyboard functionality, duplicated code, and more.

@oliviertassinari @mbrookes a few questions:

Do you think the label wrapped switch should be the default export like I have it in this PR?
In order to accomodate it being the default export, I had to make some styles conditional on an actual label being provided. If the label version stays as the default export, do you think this should stay or should none-labelled instances be using the named export instead?

cc @rosskevin:

I've removed labelReverse here. I can't find a single common use case in an RTL language to justify it being part of core. It's just single flex property so think it should be done in userland. WDYT?

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).

rosskevin · 2016-12-13T23:41:19Z

There is a labelReverse demo in Selection controls, specifically Switch. In a case where you wanted a switch with a label in a list, you would simply need to style the separation. I do think it is removable since this use case needs styling externally anyway.

nathanmarks · 2016-12-13T23:47:33Z

@rosskevin Sounds good to me--also, I don't see the common use case for it outside of the list menus anyways, and for the material list menus, we do have a component that handles the list and placement already.

rosskevin · 2016-12-13T23:50:19Z

I like the code reduction.

From regressions, it looks like components were column layout, now they are row layout. I've been meaning to PR, but this is a good time to discuss and get it in. I'd like consistency of FormGroup and RadioGroup with behavior of the row property. I suggest default column layout and boolean row will inline controls.

Does this sound like a consistent/good addition to RadioGroup?

nathanmarks · 2016-12-14T00:08:22Z

@rosskevin I think RadioGroup has been misunderstood, the reason for RadioGroup initially is because with radios it's handy to have an interface similar to a <select> (they are fairly interchangeable in practice). It was never intended as a layout tool and it doesn't have opinions on layout.

I didn't change anything that was explicitly setting a column layout. What I did do was change the display from flex to inline-flex so that the layout was consistent with the switches that didn't have a label. This was only happening because of the inconsistent display property between the two:

I suppose my question is, why should column be the default for selection controls? And if we are doing that, we should enforce it via flex-direction of the parent like FormGroup does.

Also, at the moment, FormGroup seems like a generic layout helper rather than something form specific. Do you think that there's anything specific to forms that it will need in the near future? Otherwise it's probably a sign that we should start thinking about a more all-purpose flex layout helper. I know there was a PR for one recently, I haven't had a chance to thoroughly check it out though.

mbrookes · 2016-12-14T00:09:03Z

@nathanmarks I'm leaning towards labelled as the default export - my gut feeling is that unlabelled switches, such as the checkboxes in Table are less common. Using the named export for non-labeled instances seems logical, rather than conditional styles, but I'll admit I don't fully understand the implications of that change.

nathanmarks · 2016-12-14T00:13:10Z

@mbrookes the labelled version actually works in tables, lists etc 😉 Just a tiny overhead by using it.

rosskevin · 2016-12-14T00:18:15Z

FormGroup is intended to be similar to RadioGroup. It is currently stubbed as and provides column vs row layout. I was planning to enhance it similar to RadioGroup specifically for integration with a FormLabel. Checkboxes often need a label (with required styling behavior, which requires knowledge of focus).

FormGroup and RadioGroup can be agnostic of layout; I would like them to behave the same out of the box. The only argument of being layout agnostic is sacrificing convenience. Row vs column layout of selection controls is incredibly common, and I am one who would be wrapping them so I could get this behavior in a simple fashion.

nathanmarks · 2016-12-14T00:20:54Z

@rosskevin I wasn't saying getting rid of the helper, I was just wondering about whether it had to be specific to forms or not. It looks super generic and could easily be made a HoC to give flex children styling to any component/element.

RadioGroup shouldn't have something that is "similar" because the primary goal of RadioGroup is to provide API helpers that aren't required for checkboxes or toggle switches. Layout is a separate concern (so FormGroup, etc)

rosskevin · 2016-12-14T00:23:43Z

@nathanmarks - it is generic now, not generic later. Make sense?

nathanmarks · 2016-12-14T00:27:17Z

@rosskevin 100%, what did you have in mind for it? As in--what other layout problems for the forms is it going to help fix? Just curious/interested

rosskevin · 2016-12-14T00:35:55Z

Did you miss my comment?

FormGroup is intended to be similar to RadioGroup...I was planning to enhance it similar to RadioGroup specifically for integration with a FormLabel. Checkboxes often need a label (with required styling behavior, which requires knowledge of focus).

I have a wrapper around RadioGroup locally that integrates FormLabel. Would like to get this into core and do the same for FormGroup. Sorry if I confused you.

nathanmarks · 2016-12-14T00:37:38Z

@rosskevin Ahhh sorry. Missed the bit about the integration RE FormLabel.

I think we need more flexibility here to keep concerns separated. What if FormGroup had a named export too, createFormGroup that you could use to wrap other components if needed? (Such as RadioGroup).

rosskevin · 2016-12-14T00:39:13Z

Agree with that. FormGroup should be able to wrap FormLabel + RadioGroup and achieve the required spec behaviors for labels.

nathanmarks · 2016-12-14T00:41:57Z

Nice, that way RadioGroup can stay focused in purpose and the code behind FormGroup can handle that concern easily for all the form groups in one place.

rosskevin · 2016-12-14T00:48:32Z

I had FormLabel focus behavior was in RadioGroup and I had it in the demo, but it was removed in the last change to RadioGroup by @oliviertassinari (prior to this pr). I see in the demo that the FormLabel is also now outside the RadioGroup.

Now label behavior is broken as I had coded it. The following label should be accented in this scenario (I clicked on the button):

@nathanmarks your idea would solve this in a more elegant way (and leave @oliviertassinari's change as-is)

nathanmarks · 2016-12-14T00:50:28Z

@rosskevin The problem we had is that with the current RadioGroup and the JS based focus management, having the label inside completely broke the ability to tab into the widget and navigate through the radios. That's why it got removed from the demo.

It should be super super easy to solve once this PR is ready. I'll try the idea I outlined above in a follow up PR.

- Refactors SwitchBase into a factory - Refactors Checkbox, Radio, Switch to use new Factory - Drastically simplify RadioGroup

nathanmarks · 2016-12-14T17:27:49Z

@oliviertassinari let me know what you think

oliviertassinari · 2016-12-14T21:44:39Z

Do you think the label wrapped switch should be the default export like I have it in this PR?

In order to accommodate it being the default export, I had to make some styles conditional on an actual label being provided. If the label version stays as the default export, do you think this should stay or should none-labelled instances be using the named export instead?

The export could be confusing right now. Doing a default import is gonna have the label property while doing a name import isn't. I like the idea of exporting by default the lower level component Checkbox and proposing a named export like CheckboxLabel with the withSwitchLabel() HoC.

Otherwise it's probably a sign that we should start thinking about a more all-purpose flex layout helper. I know there was a PR for one recently, I haven't had a chance to thoroughly check it out
though.

I had a close look at them. For what it worth, at @doctolib, we have been developing and using a simple homemade solution based on the #3614 discussion: LayoutBlock (still wip).

oliviertassinari

Cool to see how well the browser is taking care of the accessibility with the native input 👍

oliviertassinari · 2016-12-14T18:38:20Z

.eslintrc.js

@@ -28,6 +28,7 @@ module.exports = {
    'no-console': 'error', // airbnb is using warn
    'no-param-reassign': 'off',
    'no-prototype-builtins': 'off',
+    'no-use-before-define': ['error', { 'functions': false }], // airbnb have functions: true, annoying


I'm curious about the motivation.

@oliviertassinari I wanted to put all the test helpers below the main part of the suite in one case. Functions are hoisted so there isn't any harm and in some scenarios it helps you leave more important stuff at the top. It isn't something that affects our react components because for the most part we have non-hoistables (class, assignment).

Sounds like a fight between important stuff at the top vs load the context before digging into the code. I prefer the second option but some flexibility won't harm 👍 .

Yeah, I see that side too -- but on the other hand it can be confusing to open a file and it's hard to find the root describe() in a test because there's a bunch of helper functions first. In that case, you don't even know what they are for yet.

oliviertassinari · 2016-12-14T18:39:55Z

src/IconButton/IconButton.js

@@ -62,6 +62,7 @@ export const styleSheet = createStyleSheet('IconButton', (theme) => {
 export default function IconButton(props, context) {
  const {
    accent,
+    buttonRef,


oliviertassinari · 2016-12-14T18:40:47Z

src/Radio/RadioGroup.js

@@ -1,34 +1,8 @@
-// @flow weak
+  // @flow weak


oliviertassinari · 2016-12-14T18:43:10Z

src/internal/ButtonBase.js

@@ -121,6 +103,8 @@ export default class ButtonBase extends Component {
  button = null;
  keyboardFocusTimeout = undefined;

+  focus = () => this.button.focus();


I think that we could have issue with this.button being null when unmounting the tree. Not sure through. Will see in the long run.

@oliviertassinari we could even remove this... although it's worth considering if it should be part of the public API so there is a react-based way to programatically focus our buttons. Unless you think that we should just recommend findDOMNOde for now. WDYT?

I don't know if findDOMNOde is going to be removed or not.
I'm not sure how we are going to document that focus imperative method.
I don't have much opinion. I would keep it for now as people might hit the eslint warning.

@oliviertassinari I don't think it's being removed -- the question is more about what the right API to expose to our users is.

I think that the imperative API is the best one. It's simpler to use to deprecated, to warn when not used correctly, it abstract the underlying dom implementation.

oliviertassinari · 2016-12-14T21:26:57Z

src/internal/SwitchLabel.js

+  const switchComponentName = SwitchComponent.displayName || SwitchComponent.name;
+
+  return class SwitchLabel extends Component {
+    static displayName = `withSwitchLabel(${switchComponentName})`;


Could be using the recompose helper to set the display name:
https://github.com/acdlite/recompose/blob/master/src/packages/recompose/createHelper.js

nathanmarks · 2016-12-15T02:45:58Z

@oliviertassinari I'm going to use LabelRadio, etc, as RadioLabel sounds too much like "label to use with the radio" rather than "radio wrapped with a label"

nathanmarks · 2016-12-15T03:49:14Z

Just noticed a small focus related issue somewhere else that switches are used (lists). Fixing that.

nathanmarks · 2016-12-15T16:40:28Z

@rosskevin I ended up leaving FormGroup as is for now and i'm actually using it inside RadioGroup as the root node.

rosskevin · 2016-12-15T18:42:47Z

@nathanmarks are you still going to pr a change to FormGroup and docs/site/src/demos/selection-controls/RadioButtonsGroup.js so that the FormLabel gets focus again or do you want me to do that?

nathanmarks added the in progress label Dec 13, 2016

oliviertassinari added the next label Dec 13, 2016

muibot added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 13, 2016

nathanmarks force-pushed the fix-radiogroup-keybd-selection branch from 9f9f56a to a31f23d Compare December 13, 2016 23:48

muibot added the PR: Needs Review label Dec 13, 2016

nathanmarks force-pushed the fix-radiogroup-keybd-selection branch 2 times, most recently from e6f45d9 to 6c0c097 Compare December 14, 2016 01:07

Refactor switches, simplify code, fix related bugs.

081c0a9

- Refactors SwitchBase into a factory - Refactors Checkbox, Radio, Switch to use new Factory - Drastically simplify RadioGroup

nathanmarks force-pushed the fix-radiogroup-keybd-selection branch from 6c0c097 to 58b47fd Compare December 14, 2016 17:17

nathanmarks changed the title ~~[wip] Refactor switches, simplify code, fix related bugs.~~ Refactor switches, simplify code, fix related bugs. Dec 14, 2016

nathanmarks removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI in progress labels Dec 14, 2016

Support keyboard focus for contained elements

bab90d4

nathanmarks force-pushed the fix-radiogroup-keybd-selection branch from 58b47fd to bab90d4 Compare December 14, 2016 17:19

oliviertassinari reviewed Dec 14, 2016

View reviewed changes

Swap default/label exports for switches

1a532a0

nathanmarks added 2 commits December 14, 2016 21:48

Remove extra whitespace

e4592fa

Use createHelper

cd1978a

nathanmarks force-pushed the fix-radiogroup-keybd-selection branch from 6da2cf8 to cd1978a Compare December 15, 2016 03:15

nathanmarks added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Dec 15, 2016

nathanmarks added 2 commits December 15, 2016 10:02

Fix demos using wrong components

5427bd6

Fix radiogroup imperative focusing for dialog menu accessibility

69a529b

muibot added the PR: Needs Review label Dec 15, 2016

Add test for radiogroup focus helper

9710496

nathanmarks removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 15, 2016

nathanmarks merged commit 3a155b1 into mui:next Dec 15, 2016

nathanmarks deleted the fix-radiogroup-keybd-selection branch December 15, 2016 16:37

zannager added the component: radio This is the name of the generic UI component, not the React module! label Mar 14, 2023

Refactor switches, simplify code, fix related bugs. #5780

Refactor switches, simplify code, fix related bugs. #5780

Conversation

nathanmarks commented Dec 13, 2016 • edited Loading

rosskevin commented Dec 13, 2016

nathanmarks commented Dec 13, 2016

rosskevin commented Dec 13, 2016

nathanmarks commented Dec 14, 2016 • edited Loading

mbrookes commented Dec 14, 2016

nathanmarks commented Dec 14, 2016 • edited Loading

rosskevin commented Dec 14, 2016

nathanmarks commented Dec 14, 2016 • edited Loading

rosskevin commented Dec 14, 2016

nathanmarks commented Dec 14, 2016

rosskevin commented Dec 14, 2016 • edited Loading

nathanmarks commented Dec 14, 2016

rosskevin commented Dec 14, 2016

nathanmarks commented Dec 14, 2016

rosskevin commented Dec 14, 2016

nathanmarks commented Dec 14, 2016 • edited Loading

nathanmarks commented Dec 14, 2016

oliviertassinari commented Dec 14, 2016 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanmarks Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanmarks commented Dec 15, 2016

nathanmarks commented Dec 15, 2016

nathanmarks commented Dec 15, 2016

rosskevin commented Dec 15, 2016

nathanmarks commented Dec 13, 2016 •

edited

Loading

nathanmarks commented Dec 14, 2016 •

edited

Loading

nathanmarks commented Dec 14, 2016 •

edited

Loading

nathanmarks commented Dec 14, 2016 •

edited

Loading

rosskevin commented Dec 14, 2016 •

edited

Loading

nathanmarks commented Dec 14, 2016 •

edited

Loading

oliviertassinari commented Dec 14, 2016 •

edited

Loading

nathanmarks Dec 15, 2016 •

edited

Loading