[Toggle Button] Implemented new component for v1-beta (Issue #2863) #7551

xsh2047 · 2017-07-26T01:40:48Z

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

This is a pull request for the Toggle Button Component specified in the Material UI Design Specifications. So far there are no tests written for this component however I would like to change that with feedback from you, the contributors.

There is a demo for each version of the component included within the documentation. This is apart of the #2863 Issue and is up-to-date with the v1-beta branches changes as of 7/25/2017.

…nto toggle-button

…to toggle-button

xsh2047 · 2017-07-26T04:53:57Z

The only lint errors are from another components comment as well as: react/no-array-index-key
which doesn't seem breaking for a component of this nature.

kgregory

This is a very nice implementation, great first contribution. I have a few notes on it and I'm hoping to open a discussion so that we can get this into the package.

kgregory · 2017-07-31T20:56:25Z

src/ToggleButton/Option.js

+  },
+});
+
+class Option extends Component {


This component is part of the ToggleButton implementation and should have a name that reflects that, similar to List having ListItem, ListItemIcon, etc. The spec indicates the following:

Toggle buttons may be used to group related options.

Maybe ToggleButton and ToggleButtonOption?

How about ToggleOption?

I vote for ToggleButtonOption

👍 for ToggleButtonOption.

Updated, it is now ToggleButtonOption

kgregory · 2017-07-31T21:02:19Z

src/ToggleButton/Option.js

+    } else {
+      const items = children.map((child, index) => {
+        return React.cloneElement(child, {
+          key: index,


Using index as the key is going to cause an eslint warning and is not a good idea.

Thanks for the link, i will definitely take a look.

This is in fact causing a lint error:

/tmp/material-ui/src/ToggleButton/Option.js
138:16 error Do not use Array index in keys react/no-array-index-key

kgregory · 2017-07-31T21:16:10Z

src/ToggleButton/Option.js

+
+    let optionButton;
+    let option;
+    // If there are children available, make it a Dropdown Menu.


What exactly is being toggled when a toggle button option has a dropdown menu? I'm not sure if this fits in the expected user experience of this component. This seems more like a dropdown button.

In your demo, you need to drop the option down and deselect the selected option to clear the selection. I know it is visually represented in the Toggle Button specification, but I think it was there to flesh out the example of using toggle buttons in a toolbar.

Perhaps if it were made into a segmented dropdown button and the left segment could be toggled with a click?

It doesn't seem to be a segmented drop down button within the specification therefore I would be inclined to agree with your previous statement about it possibly just being there for appearance.

All tings considered, what would the accepted functionality be?

Personally I agree with the dropdown being separated if the current implementation isn't acceptable.

What is the final verdict on this?

kgregory · 2017-07-31T21:24:19Z

src/ToggleButton/Option.js

+    color: fade(common.black, 0.3),
+  },
+  textSelected: {
+    color: fade(common.black, 0.54),


When using toggleIcon to present icons as toggle button options, the only indication that an icon has been selected is that the color changes slightly. In the specification, the color of the icon is the same for normal, hover, focus, and pressed states. The visual cue that an icon is in the pressed state is its radial background, which is a function of the icon's color.

The icon toggle focus indicator color and opacity are related to the color of the icon.

For this to work for icons, there will have to be some work done to the background. It would be nice to support a disabled icon as well. Perhaps through a type property, similar to the one implemented for button (primary, accent, disabled, contrast, default).

I will be updating the component accordingly thanks for that note.

For a component such as the Toggle Icon, would it make sense to have the user pass a color directly? Or utilize one from the theme?

Users can always override the value, but I think that we need a default value.

a variation of color as suggested by @kgregory sounds good too.

kgregory · 2017-07-31T21:25:46Z

docs/src/pages/component-demos/toggle-button/NormalToggleButton.js

+          <MenuItem value={5}>Blue</MenuItem>
+          <MenuItem value={6}>Green</MenuItem>
+        </Option>
+      </ToggleButton>


I don't think this should be part of the demo because it doesn't seem to be in the scope of a toggle button's user experience. I think this is more like a dropdown button.

It could work if this were a segmented dropdown button and the left segment could be used to toggle the selection.

Hopefully we can open a discussion on this.

Hopefully, based on the discussion for the previously raised query we can resolve this.

kgregory · 2017-08-03T14:36:44Z

src/internal/SwitchBase.js

+   /**
+   ** NB: If changed, please update Checkbox, Switch and Radio
+   ** so that the API documentation is updated.
+   **/


This comment block is causing one of your lint errors:

/tmp/material-ui/src/internal/SwitchBase.js
158:3 error Delete · prettier/prettier
158:4 error Expected space or tab before '*/' in comment spaced-comment

Since the next comment is identical, you should remove it.

This is a duplication, it should be removed.

kgregory · 2017-08-03T14:37:40Z

src/ToggleButton/Option.js

+    } else {
+      const items = children.map((child, index) => {
+        return React.cloneElement(child, {
+          key: index,


This is in fact causing a lint error:

/tmp/material-ui/src/ToggleButton/Option.js
138:16 error Do not use Array index in keys react/no-array-index-key

oliviertassinari

aside from the code, some issues I have noticed with the component:

focus feedback issue
changing width when selected
docs/yarn.lock is needed
No contrast with the dark theme.
Some ripple issue, for instance, it can no longer be seen on the Table page

oliviertassinari · 2017-08-03T19:34:11Z

docs/src/pages/component-demos/toggle-button/ExclusiveToggleButton.js

+import Paper from 'material-ui/Paper';
+import Typography from 'material-ui/Typography';
+
+const styleSheet = createStyleSheet('ExclusiveToggleButton', {


You do no longer need to provide the sheet name. We have removed it from all our demo example. Please do the same here :)

oliviertassinari · 2017-08-03T19:34:42Z

docs/src/pages/component-demos/toggle-button/ExclusiveToggleButton.js

+    width: '100%',
+  },
+  paper: {
+    padding: 30,


using a multiplier of theme.spacing.unit would be better.

oliviertassinari · 2017-08-03T19:35:56Z

docs/src/pages/component-demos/toggle-button/ExclusiveToggleButton.js

+function ExclusiveToggleButton(props) {
+  const classes = props.classes;
+
+  function alignRight() {


Can't those functions be defined in the parent scope? The issue right not is that they are going to be created at each render.

oliviertassinari · 2017-08-03T19:36:40Z

docs/src/pages/component-demos/toggle-button/ExclusiveToggleButton.js

+          onDeselect={alignReset}
+        />
+      </ToggleButton>
+


We almost never use blank line between the components.

oliviertassinari · 2017-08-03T21:00:35Z

docs/src/pages/component-demos/toggle-button/toggle-button.md

+
+{{demo='pages/component-demos/toggle-button/ToggleIcons.js'}}
+
+


Extra lines

oliviertassinari · 2017-08-03T21:20:53Z

src/ToggleButton/ToggleButton.js

+
+  state = {
+    active: false,
+    values: [],


That state is not used in the render method, no need.

Please elaborate

It can be an instance property. But then, it feels wrong to have this property at all. We need to remove it.

oliviertassinari · 2017-08-03T21:23:09Z

src/ToggleButton/ToggleButton.js

+  /**
+   * Values of the currently selected options on the 'ToggleButton'.
+   */
+  values: PropTypes.arrayOf(


I don't think that we need this property.

oliviertassinari · 2017-08-03T21:23:38Z

src/ToggleButton/ToggleButton.js

+  /**
+   * Indexes of the currently selected options on the 'ToggleButton'.
+   */
+  selectedOptions: PropTypes.arrayOf(PropTypes.number),


What about calling that property value?

It doesn't fit with what's stored

We use the value/onChange` couple as much as we can for controlling components. How doesn't that match what's store?

oliviertassinari · 2017-08-03T21:24:16Z

src/ToggleButton/Option.js

+   *
+   * @param {number, boolean, string} value The current value of the selected option.
+   */
+  onDeselect: PropTypes.func,


What about merging onDeselect and onSelect into onChange?

oliviertassinari · 2017-08-03T21:24:36Z

src/ToggleButton/ToggleButton.js

+  };
+
+  state = {
+    active: false,


I don't think that we need that state. As far as I can see it, it can be extracted from the selectedOptions property.

oliviertassinari · 2017-08-03T21:28:59Z

@zabieru96 I'm happy to see this PR :).
To put it up-front, it's gonna require some work before we can merge it. Expect it to need a good day full-time.

…pecification

…a disabled prop

oliviertassinari · 2017-08-05T21:07:15Z

This seems more like a dropdown button.

@kgregory I agree, it seems to be used in a broader context than the ToggleButton. I would be fine with the DropdownButton wording.

your previous statement about it possibly just being there for appearance

@zabieru96 I don't think so, I think that we have to deal with a specific type of DropdownButton. We could implement if from the Paper and the MenuItem components. Something close to the Autocomplete demo. I don't think that we can use the Menu component as it's now. It's covering and hiding the element opening it. We have different options. Either we write an abstraction like the Menu to deal with it, or we expose an implementation in the demo with the Popover. If people want to, they should be able to use another positioning library like popper.js, react-popper.
That's some thoughts I have. I haven't been digging more into it.

…utton's parent component

mbrookes · 2017-08-07T15:48:55Z

Appreciate this is still a WIP, but please could you update https://github.com/callemall/material-ui/blob/v1-beta/docs/src/pages/getting-started/supported-components.md as part of this PR?

stevewillard · 2017-08-17T02:05:57Z

What's the status of this PR? Would love to use this component. Thanks for putting it together!

… the specification and setup basic tests

…on non-exclusive toggle button

xsh2047 · 2017-08-23T18:42:15Z

To clarify should i remove the dropdown functionality and example? Also @stevewillard I have a few test cases i'm writing and would require further feedback.

xsh2047 · 2017-08-25T10:14:40Z

How do i fix the size limit error?
I'm over by like .75 KB.
Should I just increase it?

oliviertassinari · 2017-08-26T22:34:33Z

@zabieru96 Have a look at the size-limit key in the package.json.

xsh2047 · 2017-08-26T23:35:14Z

@oliviertassinari I'm aware of the key, I'm just not sure if it's advised that I change it.

oliviertassinari · 2017-08-27T10:08:01Z

@zabieru96 You can definitely increase the value of the size-limit. It's 100% fine for new components. I'm having a look at this PR now :).

oliviertassinari

Some more feedbacks. I haven't gone through all the changes of this PR. Some other issues I have noticed

Missing focus feedback
Toggle icon style no following the specs
Not vertically centered icon
Stateful components. I do think that they should all be stateless.
Missing accessibility
Need to rebase, we refactored the style handling and the documentation.

If you don't want to go through all those changes, I can understand and we can close the PR.

Thanks again for pushing Material-UI forward!

oliviertassinari · 2017-08-27T10:14:38Z

docs/src/pages/component-demos/toggle-button/ExclusiveToggleButton.js

+function alterText(value, selected) {
+  const div = document.getElementById('dummyDiv2');
+  if (selected && div !== null) {
+    if (value === '1') {


I think that a switch case would be better.

oliviertassinari · 2017-08-27T10:15:04Z

docs/src/pages/component-demos/toggle-button/ExclusiveToggleButton.js

+}));
+
+function alterText(value, selected) {
+  const div = document.getElementById('dummyDiv2');


Why not using react for this task?

How do you propose it be done?

oliviertassinari · 2017-08-27T10:15:28Z

docs/src/pages/component-demos/toggle-button/ExclusiveToggleButton.js

+    <div className={classes.root}>
+      <ToggleButton exclusive className="toggle">
+        <ToggleButtonOption
+          key="1"


What's the key is for? I don't think that you need it.

A key has to be specified on the option because i utilize the map function within the ToggleButton component.

oliviertassinari · 2017-08-27T10:15:50Z

docs/src/pages/component-demos/toggle-button/ExclusiveToggleButton.js

+
+  return (
+    <div className={classes.root}>
+      <ToggleButton exclusive className="toggle">


What's the className is for? I don't think that you need it.

It's so that the paper is full width, without it it's width only spans it's contents.

oliviertassinari · 2017-08-27T10:19:46Z

src/internal/TouchRipple.js

@@ -15,7 +15,7 @@ export const styleSheet = createStyleSheet('MuiTouchRipple', theme => ({
  root: {
    display: 'block',
    position: 'absolute',
-    overflow: 'hidden',
+    // overflow: 'hidden', The ripple should be limited by an external component in my opinion.


This is breaking the other components. I don't see how it's related to this PR. Please add it back. We can discuss it aside from this PR.

oliviertassinari · 2017-08-27T10:20:03Z

src/ToggleButton/ToogleButton.spec.js

+    it('should render with toggleIcon class', () => {
+      const wrapper = shallow(
+        <ToggleButton toggleIcons>
+          <ToggleButtonOption key="1" />


No need for the key.

oliviertassinari · 2017-08-27T10:20:24Z

src/ToggleButton/ToggleButtonOption.spec.js

+    });
+  });
+
+  describe('prop : divider', () => {


We use prop: without the space.

oliviertassinari · 2017-08-27T10:23:11Z

src/ToggleButton/ToggleButton.js

+  /**
+   * Indexes of the currently selected options on the 'ToggleButton'.
+   */
+  selectedOptions: PropTypes.arrayOf(PropTypes.number),


We use the value/onChange` couple as much as we can for controlling components. How doesn't that match what's store?

oliviertassinari · 2017-08-27T10:23:57Z

src/ToggleButton/ToggleButtonOption.js

+          color: selected ? fade(common.black, 0.54) : fade(common.black, 0.3),
+        },
+      };
+      const menuProps = {


Please take full advantage of JSX so we write all our components the same way (reducing entropy).

It can be an instance property. But then, it feels wrong to have this property at all. We need to remove it.

Which property are you referring to?

We use the value/onChange` couple as much as we can for controlling components. How doesn't that match what's store?

Because what's stored in this property is the position of the selected item, not the value it contains.

oliviertassinari · 2017-08-27T10:26:20Z

src/ToggleButton/ToggleButton.js

+
+  state = {
+    active: false,
+    values: [],


It can be an instance property. But then, it feels wrong to have this property at all. We need to remove it.

…ed size check from 120KB to 121KB

xsh2047 · 2017-08-27T23:16:56Z

As for the Rebase I am still having trouble accessing the demos and api section of the docs site and that would mean a bottleneck until i can figure out the problem.

…to toggle-button

xsh2047 · 2017-08-28T14:45:45Z

Okay so a recent push to the repository solves my previously mentioned error. I just need to know how to migrate my component.

…to toggle-button

xsh2047 · 2017-08-28T18:27:04Z

Okay so I have updated the branch to incorporate the code in v1-beta. I guess all thats left is another review.

oliviertassinari · 2017-08-28T18:39:21Z

@zabieru96 Will give another look :)

oliviertassinari · 2017-09-12T08:21:38Z

@zabieru96 We appreciate the work you have done on this PR so far! Thanks for your time :). However, I don't think that we are near merging it, for instance the build is green and I have been raising some issues that haven't been fixed. I'm going to close it as haven't moved forward for quite some time.

xsh2047 added 13 commits June 22, 2017 02:36

[ToggleButton] Added Toggle Button Component

dfccc2c

[ToggleButton] Added Toggle Button Component

a47b717

[Toggle Button] Style changes as well as state bug fix

c511a2c

Updated with next branch

5550823

[Docs] Added Documentation for Toggle Button

fe95337

Merge branch 'v1-alpha' of https://github.com/callemall/material-ui i…

cc815c6

…nto toggle-button

Merge branch 'v1-alpha' of https://github.com/callemall/material-ui i…

6f8c560

…nto toggle-button

[Toggle Button] Changed color references

eaf0058

Merge branch 'v1-beta' of https://github.com/callemall/material-ui in…

ee1186a

…to toggle-button

[Toggle Button] Refactored for v1-beta branch

1240848

[lint] Fixed various linting errors

2b15588

Merge branch 'v1-beta' of https://github.com/callemall/material-ui in…

4ab6bc5

…to toggle-button

[lint] Fixed various linting errors

4b82ec5

xsh2047 changed the title ~~Toggle button~~ [Toggle Button] Implemented new component for v1-beta (Issue #2863) Jul 29, 2017

kgregory reviewed Jul 31, 2017

View reviewed changes

rosskevin added the v1 label Aug 2, 2017

kgregory reviewed Aug 3, 2017

View reviewed changes

oliviertassinari requested changes Aug 3, 2017

View reviewed changes

oliviertassinari 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 Aug 3, 2017

xsh2047 added 4 commits August 5, 2017 12:29

[ToggleButton] Renamed Option to ToggleButtonOption

f56d640

[ToggleButton] Changed toggle icon behaviour to fit the Material UI S…

c16be7e

…pecification

[ToggleButton] Changed noBackground prop to toggle

954d72f

[ToggleButton] Added Support for Disabling a ToggleButtonOption with …

880dda1

…a disabled prop

xsh2047 added 4 commits August 5, 2017 17:58

[ToggleButton] Adjusted the issuing of keys to be done at the ToggleB…

b744e73

…utton's parent component

[ToggleButton] Fixed lint errors

d20a80f

[ToggleButton] Up to date with v1-beta branch

ccbb003

[ToggleButton] Generated yarn.lock files

53b3bf7

xsh2047 added 2 commits August 23, 2017 12:13

[ToggleButtons] Changed Toggle Icons color selection to match that of…

ddf82fa

… the specification and setup basic tests

[ToggleButtons] Fixed extra width added after mutiple items selected …

a81cace

…on non-exclusive toggle button

[ToggleButton] Fixed lint and flow errors and wrote tests

2e1d25b

oliviertassinari added new feature New feature or request and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Aug 26, 2017

oliviertassinari requested changes Aug 27, 2017

View reviewed changes

[ToggleButton] Made changes according to feedback from PR and increas…

1d89a19

…ed size check from 120KB to 121KB

xsh2047 added 2 commits August 28, 2017 08:46

[ToggleButton] Rebase with v1-beta branch

f00bbf3

Merge branch 'v1-beta' of https://github.com/callemall/material-ui in…

3325cb8

…to toggle-button

xsh2047 added 2 commits August 28, 2017 13:23

[ToggleButton] Up-to-date with v1-beta branch

e3af98b

Merge branch 'v1-beta' of https://github.com/callemall/material-ui in…

8e3208d

…to toggle-button

xsh2047 added 2 commits August 28, 2017 13:30

[ToggleButton] Made adjustments to test case

49a07fa

[ToggleButton] Fixed lint errors

956d4ec

oliviertassinari closed this Sep 12, 2017

oliviertassinari mentioned this pull request Jan 19, 2018

IconButton: Recommended approach to showing toggle state #9941

Closed

1 task


		{{demo='pages/component-demos/toggle-button/ToggleIcons.js'}}

[Toggle Button] Implemented new component for v1-beta (Issue #2863) #7551

[Toggle Button] Implemented new component for v1-beta (Issue #2863) #7551

Conversation

xsh2047 commented Jul 26, 2017 • edited Loading

xsh2047 commented Jul 26, 2017

kgregory 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

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

kgregory Jul 31, 2017 • 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

kgregory Jul 31, 2017 • 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Aug 3, 2017 • edited Loading

oliviertassinari commented Aug 5, 2017

mbrookes commented Aug 7, 2017

stevewillard commented Aug 17, 2017

xsh2047 commented Aug 23, 2017 • edited Loading

xsh2047 commented Aug 25, 2017

oliviertassinari commented Aug 26, 2017

xsh2047 commented Aug 26, 2017

oliviertassinari commented Aug 27, 2017

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

Choose a reason for hiding this comment

xsh2047 Aug 27, 2017 • 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

xsh2047 Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xsh2047 commented Aug 27, 2017

xsh2047 commented Aug 28, 2017

xsh2047 commented Aug 28, 2017

oliviertassinari commented Aug 28, 2017

oliviertassinari commented Sep 12, 2017

xsh2047 commented Jul 26, 2017 •

edited

Loading

kgregory Jul 31, 2017 •

edited

Loading

kgregory Jul 31, 2017 •

edited

Loading

oliviertassinari commented Aug 3, 2017 •

edited

Loading

xsh2047 commented Aug 23, 2017 •

edited

Loading

xsh2047 Aug 27, 2017 •

edited

Loading

xsh2047 Aug 27, 2017 •

edited

Loading