[core] Use context to pass data between Group components and their children #14585

joshwooding · 2019-02-19T22:43:10Z

I have followed (at least) the PR section of the contributing guide.

Continuation of #12961

Todo

Tests
TypeScript
Look at RadioButtonGroup ([RadioButtonGroup] breaks when nesting non-RadioButton components #2225)
Move SelectableGroup to a better location (internal) and remove docs

eps1lon · 2019-02-20T14:21:51Z

Have you tried rethinking this whole approach with hooks in mind? I'd imagine something like a SelectGroup that acts as the controller and some deep children can then use something like const { isSelected, toogle, select, deselect } = useSelectedState()

mui-pr-bot · 2019-02-21T19:02:26Z

@material-ui/core: parsed: +0.15% , gzip: +0.23%
@material-ui/lab: parsed: +0.65% , gzip: +1.07%

Details of bundle changes.

Comparing: a5b8011...ec23622

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.15% 🔺	+0.23% 🔺	362,777	363,329	92,213	92,424
@material-ui/core/Paper	+0.01% 🔺	+0.03% 🔺	68,878	68,885	20,170	20,176
@material-ui/core/Paper.esm	+0.01% 🔺	+0.01% 🔺	63,340	63,344	19,272	19,273
@material-ui/core/Popper	0.00%	0.00%	30,460	30,460	10,573	10,573
@material-ui/core/styles/createMuiTheme	0.00%	+0.05% 🔺	17,284	17,284	5,700	5,703
@material-ui/core/useMediaQuery	-0.24%	+0.58% 🔺	2,471	2,465	1,035	1,041
@material-ui/lab	+0.65% 🔺	+1.07% 🔺	170,747	171,855	50,065	50,601
@material-ui/styles	0.00%	-0.04%	57,220	57,220	16,273	16,266
@material-ui/system	-0.04%	+0.22% 🔺	17,041	17,035	4,493	4,503
Button	-0.44%	-0.59%	91,152	90,747	26,939	26,781
Modal	-0.45%	-0.42%	90,261	89,857	26,724	26,611
colorManipulator	0.00%	0.00%	3,232	3,232	1,298	1,298
docs.landing	0.00%	0.00%	51,908	51,908	11,302	11,302
docs.main	+0.08% 🔺	+0.11% 🔺	647,808	648,302	201,557	201,775
packages/material-ui/build/umd/material-ui.production.min.js	+0.14% 🔺	+0.39% 🔺	311,179	311,604	85,197	85,526

Generated by 🚫 dangerJS against ec23622

joshwooding · 2019-02-21T20:02:05Z

@eps1lon Hooks have already been thought about 🎉 I’ve added a demo showcasing them and changed the name

Note: If we agree on the name change withSelectableContext should probably be changed too.

.eslintrc.js

packages/material-ui-lab/src/SelectableGroup/SelectableGroup.js

oliviertassinari · 2019-02-23T23:36:34Z

Can we try to use the same module with the RadioGroup component?

joshwooding · 2019-02-23T23:42:32Z

Can we try to use the same module with the RadioGroup component?

It’s on the todo list ☝️😉

oliviertassinari

I'm happy to see the migration of the tests to the mount API :)

packages/material-ui/src/RadioGroup/RadioGroup.js

packages/material-ui/src/Radio/Radio.js

packages/material-ui-lab/src/SelectableGroup/SelectableGroup.js

packages/material-ui/src/Radio/Radio.js

eps1lon

This is proper programming. How I love react these days: using patterns, functional programming for complex behavior, hiding implementation details. Makes the code so much cleaner, easier to navigate and potential optimization emerge by themselves.

Just some minor things left to discuss.

eps1lon · 2019-03-14T08:41:15Z

packages/material-ui-lab/src/ToggleButton/ToggleButton.js

@@ -7,6 +7,7 @@ import withStyles from '@material-ui/core/styles/withStyles';
 import { fade } from '@material-ui/core/styles/colorManipulator';
 import ButtonBase from '@material-ui/core/ButtonBase';
 import withForwardedRef from '@material-ui/core/utils/withForwardedRef';
+import useSelectedState from '@material-ui/core/internal/SelectableGroup/useSelectedState';


It's not really internal then. I don't mind having this as a top-level export and do

Suggested change

import useSelectedState from '@material-ui/core/internal/SelectableGroup/useSelectedState';

import { useSelectedState } from '@material-ui/core/SelectableGroup';

@oliviertassinari What do you think? (context: #14585 (comment))

@eps1lon I don't follow. I don't see anything making this module public. Why should it be made public? What do you think of keeping it private, at least, until it proves its purposes?

packages/material-ui-lab/src/ToggleButton/ToggleButton.js

packages/material-ui-lab/src/ToggleButton/ToggleButton.test.js

eps1lon · 2019-03-14T08:47:39Z

packages/material-ui-lab/src/ToggleButton/ToggleButton.test.js


-    it('should not be called if the click is prevented', () => {
-      const handleChange = spy();
+  describe('non exclusive', () => {


I think that test is better placed at SelectableGroup.

eps1lon · 2019-03-14T08:55:57Z

packages/material-ui/src/RadioGroup/RadioGroup.test.js

-    );
-  });
-
-  describe('imperative focus()', () => {


Pretty much going on already in this PR. As a PoC this is good enough.

I'd say we

don't apply to RadioGroup yet

Convert RadioGroup to function component

add the imperative focus handle

use SelectableGroup

Point 2-4 can be combined in a single PR but should each be done in its own commit. Converting to fuction component creates a lot of diff noise.

Sounds like a plan 👍

packages/material-ui/src/internal/SelectableGroup/SelectableGroup.js

packages/material-ui/src/internal/SelectableGroup/withSelectableContext.js

packages/material-ui/src/internal/SelectableGroup/SelectableGroupContext.js

packages/material-ui/src/internal/SelectableGroup/SelectableGroup.d.ts

Co-Authored-By: joshwooding <12938082+joshwooding@users.noreply.github.com>

… into selectable-group

oliviertassinari · 2019-03-17T20:30:59Z

packages/material-ui/package.json

@@ -48,6 +48,7 @@
    "hoist-non-react-statics": "^3.2.1",
    "is-plain-object": "^2.0.4",
    "normalize-scroll-left": "^0.1.2",
+    "object-is": "^1.0.1",


Do we need a dependency for such a small module?
https://github.com/ljharb/object-is/blob/master/index.js

oliviertassinari · 2019-03-17T20:35:16Z

packages/material-ui/src/internal/SelectableGroup/SelectableGroup.js

@@ -0,0 +1,104 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+import * as utils from './utils';


What do you think of inlining the helper in this file? I'm not sure to see what we win at creating a single dependent module of 32 lines.

oliviertassinari · 2019-03-17T20:37:09Z

packages/material-ui/src/internal/SelectableGroup/SelectableGroup.js

+  }
+
+  // Only call onChange if it exists and state has changed (Object.is mimics React)
+  if (action.onChange && !is(newState.selected, state.selected)) {


What's preventing us from ignoring NaN and use === over object-js?

yarn.lock

oliviertassinari · 2019-03-17T20:40:55Z

packages/material-ui-lab/src/ToggleButton/ToggleButton.js

@@ -7,6 +7,7 @@ import withStyles from '@material-ui/core/styles/withStyles';
 import { fade } from '@material-ui/core/styles/colorManipulator';
 import ButtonBase from '@material-ui/core/ButtonBase';
 import withForwardedRef from '@material-ui/core/utils/withForwardedRef';
+import useSelectedState from '@material-ui/core/internal/SelectableGroup/useSelectedState';


@eps1lon I don't follow. I don't see anything making this module public. Why should it be made public? What do you think of keeping it private, at least, until it proves its purposes?

oliviertassinari · 2019-03-17T20:42:19Z

packages/material-ui/src/internal/SelectableGroup/useSelectedState.js

+/**
+ * @ignore - internal component.
+ */
+function useSelectedState() {


We can use React.useContext(SelectableGroupContext) directly in the components. Why abstracting it?

It's not abstraction it's readability while hiding implementation details. Just because we can doesn't mean we should.

readability, I haven't noticed the difference. My point of interest is the potential module overhead removal by overhead, I mean from a maintainer perspective: "we have x dependencies, what each dependency is doing? vs I have x - 1."
Now, I have a co-worker that "loves" (find it easier to follow) creating modules for 1 line functions. I understand the motivation.

oliviertassinari · 2019-03-17T20:44:20Z

packages/material-ui/src/RadioGroup/RadioGroup.js

-
-    return (
+  return (
+    <SelectableGroup onChange={handleChange} value={value} name={name} exclusive>


As we are using the exclusive mode. Can't we save SelectableGroup bundle size by using a context forwarding direct? I mean, I would anticipate a straightforward logic without SelectableGroup, but by using the context. Do we need this abstraction? Maybe it's only suited for the ToggleButton? I was thinking of doing something similar with the Tabs component. I'm not I guess React context is all we need?

oliviertassinari · 2019-03-17T20:46:15Z

I'm happy to see the class -> function component migration and shallow -> mount :)

oliviertassinari · 2019-03-17T20:50:41Z

@joshwooding I'm having a existancial question, I'm looking at https://github.com/palmerhq/radio-group/blob/master/src/index.tsx codebase, using the context seems straightforward. I'm wondering if we need a SelectableGroup abstraction. I mean, let's compare option A and B.
option A, we migrate the logic to use a the react context directly. option B, we use an intermediary abstraction. What do we win with B?

eps1lon · 2019-03-18T07:15:06Z

@joshwooding I'm having a existancial question, I'm looking at palmerhq/radio-group:src/index.tsx@master codebase, using the context seems straightforward. I'm wondering if we need a SelectableGroup abstraction. I mean, let's compare option A and B.
option A, we migrate the logic to use a the react context directly. option B, we use an intermediary abstraction. What do we win with B?

That has no exclusive/non-exclusive option, no uncheck action. This is why you abstract it. Not because of DRY but because there is separate behavior that is shared among different components. Why is it so important for you to use the context directly here?

It would be nice to have a concrete discussion about this and not just esoteric questions like "why not do X instead of Y?" It's your task to compare these different options not just point to X and say it's better and leave use to debunk this. That's not how a healthy discussion works.

I'm really confused why you think the code you linked is a valid comparison here? We're trying to solve this issue for ToggleButtons, RadioButtons and in the the Select components as well. That solution has only a fraction of this scope.

oliviertassinari · 2019-03-18T08:38:54Z

Why is it so important for you to use the context directly here?

@eps1lon People are reading our code base, often as a reference, people are also reporting that we overengineer stuff. The fewer layers of abstraction of code we can get away with, the simpler. So, I'm challenging the need for it, it's a potential simplification opportunity. I'm only interested in the pros & cons.

That has no exclusive/non-exclusive option, no uncheck action. This is why you abstract it. Not because of DRY but because there is separate behavior that is shared among different components.

I believe there we are doing multiple things at the same time.
The way I see it: we didn't create the abstraction in the first place when using the cloneElement API. Why? We could very well do the migration in multiple steps (pull requests).

Replace a class component with a functional component. 💯
Replace the cloneElement API with a context. 💯
Then what happened: maybe we will notice it's straightforward: replace cloneElement by a provider, replace a prop extraction by a useContext: it's a 4 line changes? (using https://github.com/palmerhq/radio-group/blob/master/src/index.tsx as a reference of what it might cost).
Recognize the opportunity for a SelectableGroup abstraction, share it with multiple components. Does is worth it?

I was the first one to encourage the usage of SelectableGroup for the RadioGroup. But, I have more context around the topic now. So I'm challenging it.

oliviertassinari · 2019-03-18T08:45:53Z

It would be nice to have a concrete discussion about this and not just esoteric questions like "why not do X instead of Y?"

@joshwooding @eps1lon I agree. What do you think of splitting the effort into 3 different steps? 1. hook, 2. context, 3. SelectableGroup. One pull request at the time.

joshwooding · 2019-03-26T22:24:52Z

I'm going to close this for now

joshwooding mentioned this pull request Feb 19, 2019

[ToggleButton, ToggleButtonGroup] Use context to pass data between ToggleButtonGroup and ToggleButton #12961

Closed

joshwooding force-pushed the selectable-group branch 2 times, most recently from 46f5689 to 3436e54 Compare February 19, 2019 23:10

joshwooding force-pushed the selectable-group branch 4 times, most recently from eac964a to daf490d Compare February 21, 2019 18:57

joshwooding force-pushed the selectable-group branch 2 times, most recently from c4a0ede to 0cae737 Compare February 21, 2019 19:11

joshwooding added component: toggle button This is the name of the generic UI component, not the React module! new feature New feature or request labels Feb 21, 2019

joshwooding force-pushed the selectable-group branch 3 times, most recently from d53bd28 to d113351 Compare February 23, 2019 21:05

oliviertassinari reviewed Feb 23, 2019

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

packages/material-ui-lab/src/SelectableGroup/SelectableGroup.js Outdated Show resolved Hide resolved

joshwooding force-pushed the selectable-group branch 8 times, most recently from eac851c to d9a3e80 Compare February 25, 2019 00:23

joshwooding requested a review from oliviertassinari February 25, 2019 00:26

oliviertassinari reviewed Feb 26, 2019

View reviewed changes

joshwooding added 7 commits March 10, 2019 16:55

Review comments

a9f39a4

Fix uncontrolled behaviour

72e41db

Rever AppWrapper change

736eeff

Fix tests and broken rebase and run prettier

16b3c3e

Fix SwitchBase Radio test and start fixing others

8c82693

Fix tests

6166150

Tidy Up and move SelectableGroup

23bea2d

joshwooding force-pushed the selectable-group branch from eb4f195 to 23bea2d Compare March 10, 2019 16:56

joshwooding changed the title ~~[ToggleButton, ToggleButtonGroup] Use context to pass data between ToggleButtonGroup and ToggleButton~~ [core] Use context to pass data between Group components and their children Mar 10, 2019

joshwooding added core and removed component: toggle button This is the name of the generic UI component, not the React module! labels Mar 10, 2019

Remove objectIs and add ponyfill

90ad836

joshwooding marked this pull request as ready for review March 11, 2019 19:18

eps1lon requested changes Mar 14, 2019

View reviewed changes

eps1lon reviewed Mar 14, 2019

View reviewed changes

packages/material-ui/src/internal/SelectableGroup/SelectableGroupContext.js Outdated Show resolved Hide resolved

packages/material-ui/src/internal/SelectableGroup/SelectableGroup.d.ts Outdated Show resolved Hide resolved

eps1lon and others added 5 commits March 14, 2019 21:59

Update SelectableGroup value prop typing

ec23622

Co-Authored-By: joshwooding <12938082+joshwooding@users.noreply.github.com>

eps1lon review

2048e9f

Merge branch 'selectable-group' of github.com:joshwooding/material-ui…

6329636

… into selectable-group

Merge branch 'next' into selectable-group

4e6a84e

Fix yarn.lock

12e2740

oliviertassinari reviewed Mar 17, 2019

View reviewed changes

eps1lon mentioned this pull request Mar 18, 2019

[MenuList] Convert to a function component #14865

Merged

1 task

joshwooding closed this Mar 26, 2019

joshwooding deleted the selectable-group branch June 11, 2019 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Use context to pass data between Group components and their children #14585

[core] Use context to pass data between Group components and their children #14585

joshwooding commented Feb 19, 2019 •

edited

Loading

eps1lon commented Feb 20, 2019

mui-pr-bot commented Feb 21, 2019 •

edited

Loading

joshwooding commented Feb 21, 2019 •

edited

Loading

oliviertassinari commented Feb 23, 2019

joshwooding commented Feb 23, 2019

oliviertassinari left a comment

eps1lon left a comment

eps1lon Mar 14, 2019

joshwooding Mar 14, 2019

oliviertassinari Mar 17, 2019 •

edited

Loading

eps1lon Mar 14, 2019

eps1lon Mar 14, 2019

joshwooding Mar 14, 2019

oliviertassinari Mar 17, 2019

oliviertassinari Mar 17, 2019

oliviertassinari Mar 17, 2019

oliviertassinari Mar 17, 2019 •

edited

Loading

oliviertassinari Mar 17, 2019 •

edited

Loading

eps1lon Mar 18, 2019

oliviertassinari Mar 18, 2019 •

edited

Loading

oliviertassinari Mar 17, 2019

oliviertassinari commented Mar 17, 2019

oliviertassinari commented Mar 17, 2019 •

edited

Loading

eps1lon commented Mar 18, 2019 •

edited

Loading

oliviertassinari commented Mar 18, 2019 •

edited

Loading

oliviertassinari commented Mar 18, 2019 •

edited

Loading

joshwooding commented Mar 26, 2019

	import useSelectedState from '@material-ui/core/internal/SelectableGroup/useSelectedState';
	import { useSelectedState } from '@material-ui/core/SelectableGroup';

[core] Use context to pass data between Group components and their children #14585

[core] Use context to pass data between Group components and their children #14585

Conversation

joshwooding commented Feb 19, 2019 • edited Loading

eps1lon commented Feb 20, 2019

mui-pr-bot commented Feb 21, 2019 • edited Loading

joshwooding commented Feb 21, 2019 • edited Loading

oliviertassinari commented Feb 23, 2019

joshwooding commented Feb 23, 2019

oliviertassinari left a comment

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Mar 17, 2019 • 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

oliviertassinari Mar 17, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Mar 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Mar 17, 2019

oliviertassinari commented Mar 17, 2019 • edited Loading

eps1lon commented Mar 18, 2019 • edited Loading

oliviertassinari commented Mar 18, 2019 • edited Loading

oliviertassinari commented Mar 18, 2019 • edited Loading

joshwooding commented Mar 26, 2019

joshwooding commented Feb 19, 2019 •

edited

Loading

mui-pr-bot commented Feb 21, 2019 •

edited

Loading

joshwooding commented Feb 21, 2019 •

edited

Loading

oliviertassinari Mar 17, 2019 •

edited

Loading

oliviertassinari Mar 17, 2019 •

edited

Loading

oliviertassinari Mar 17, 2019 •

edited

Loading

oliviertassinari Mar 18, 2019 •

edited

Loading

oliviertassinari commented Mar 17, 2019 •

edited

Loading

eps1lon commented Mar 18, 2019 •

edited

Loading

oliviertassinari commented Mar 18, 2019 •

edited

Loading

oliviertassinari commented Mar 18, 2019 •

edited

Loading