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

[InputBase] Remove legacy lifecycle methods #13487

Merged
merged 3 commits into from
Nov 5, 2018
Merged

[InputBase] Remove legacy lifecycle methods #13487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe2ecef
[InputBase] Remove legacy lifecycle methods
eps1lon Nov 1, 2018
d9ff717
[InputBase] Fix focused state not being updated when context changed
eps1lon Nov 1, 2018
7f4c556
[FormControl] Add tests for gDSFP
eps1lon Nov 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/material-ui/src/FormControl/FormControl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ export const styles = {
* ⚠️ Only one input can be used within a FormControl.
*/
class FormControl extends React.Component {
static getDerivedStateFromProps(props, state) {
Copy link
Member

@oliviertassinari oliviertassinari Nov 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it's the InputBase that should take care of it by calling handleBlur. Why do we need this logic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

- <FormControl disabled={false}><InputBase /></FormControl>
+ <FormControl disabled={true}><InputBase /></FormControl>

InputBase already calls handleBlur separately if it's own focused state changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow, the disable change will propagate down to the input.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need an integration test? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's context not props propagation. If you think that some behavior was already covered feel free to add a test. Especially focused was not considered correctly.

But why should InputBase call onBlur in it's parent when the parent got changed and can take care of itself? No need to delegate that logic into the child.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The form control should propagate the disabled state to the input with the context. The input can then collect it. This way, we can avoid logic duplication. I think that we should remove the old context usage at the same time this logic is very dependant on the context implementation. I think that it will be simpler and less error prone to do both at the same time.

Yeah, it would be great to have an integration tests. It's tricky. Well, I'm also fine with logic duplication if it's simpler.

Copy link
Member Author

@eps1lon eps1lon Nov 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only logic duplication if you have a mental model with the old context and lifecycle methods. I don't think the discussion is helpful here. If you know how to access context in gDSFP or cDU I'd be happy to change it but I don't know how.

this logic is very dependant on the context implementation.

I can only work with what's been given to me. InputBase was always tightly coupled with the context.

Copy link
Member

@oliviertassinari oliviertassinari Nov 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gDSFP or cDU I'd be happy to change it but I don't know how.

We can't, it's why I'm raising the new context api, where we would be injecting the context as properties.

if (props.disabled && state.focused) {
return { focused: false };
}
return null;
}

constructor(props) {
super();

Expand Down
9 changes: 9 additions & 0 deletions packages/material-ui/src/FormControl/FormControl.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,15 @@ describe('<FormControl />', () => {
});
});

describe('prop: disabled', () => {
it('will be unfocused if it gets disabled', () => {
const wrapper = shallow(<FormControl />);
wrapper.setState({ focused: true });
wrapper.setProps({ disabled: true });
assert.strictEqual(wrapper.state().focused, false);
});
});

describe('input', () => {
it('should be filled with a value', () => {
const wrapper = shallow(
Expand Down
59 changes: 21 additions & 38 deletions packages/material-ui/src/InputBase/InputBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,47 +154,21 @@ export function formControlState({ props, states, context }) {
* It contains a load of style reset and some state logic.
*/
class InputBase extends React.Component {
static getDerivedStateFromProps(props, state) {
// The blur won't fire when the disabled state is set on a focused input.
// We need to book keep the focused state manually.
if (props.disabled && state.focused) {
return { focused: false };
}
return null;
}

constructor(props, context) {
super(props, context);
this.isControlled = props.value != null;
if (this.isControlled) {
this.checkDirty(props);
}

const componentWillReceiveProps = (nextProps, nextContext) => {
// The blur won't fire when the disabled state is set on a focused input.
// We need to book keep the focused state manually.
if (
!formControlState({ props: this.props, context: this.context, states: ['disabled'] })
.disabled &&
formControlState({ props: nextProps, context: nextContext, states: ['disabled'] }).disabled
) {
this.setState({
focused: false,
});
}
};

const componentWillUpdate = (nextProps, nextState, nextContext) => {
// Book keep the focused state.
if (
!formControlState({ props: this.props, context: this.context, states: ['disabled'] })
.disabled &&
formControlState({ props: nextProps, context: nextContext, states: ['disabled'] }).disabled
) {
const { muiFormControl } = this.context;
if (muiFormControl && muiFormControl.onBlur) {
muiFormControl.onBlur();
}
}
};

/* eslint-disable no-underscore-dangle */
this.componentWillReceiveProps = componentWillReceiveProps;
this.componentWillReceiveProps.__suppressDeprecationWarning = true;
this.componentWillUpdate = componentWillUpdate;
this.componentWillUpdate.__suppressDeprecationWarning = true;
/* eslint-enable no-underscore-dangle */
}

state = {
Expand All @@ -215,7 +189,14 @@ class InputBase extends React.Component {
}
}

componentDidUpdate() {
componentDidUpdate(prevProps) {
// Book keep the focused state.
if (!prevProps.disabled && this.props.disabled) {
const { muiFormControl } = this.context;
if (muiFormControl && muiFormControl.onBlur) {
muiFormControl.onBlur();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something that should go to the chopping block in a major. Why would someone put the component into the context if he then controls it via props on InputBase rather than props on `FormControl``

}
}
if (this.isControlled) {
this.checkDirty(this.props);
} // else performed in the onChange
Expand Down Expand Up @@ -354,13 +335,15 @@ class InputBase extends React.Component {
states: ['disabled', 'error', 'margin', 'required', 'filled'],
});

const focused = muiFormControl ? muiFormControl.focused : this.state.focused;

const className = classNames(
classes.root,
{
[classes.disabled]: fcs.disabled,
[classes.error]: fcs.error,
[classes.fullWidth]: fullWidth,
[classes.focused]: this.state.focused,
[classes.focused]: focused,
[classes.formControl]: muiFormControl,
[classes.marginDense]: fcs.margin === 'dense',
[classes.multiline]: multiline,
Expand Down Expand Up @@ -424,7 +407,7 @@ class InputBase extends React.Component {
? renderPrefix({
...fcs,
startAdornment,
focused: this.state.focused,
focused,
})
: null}
{startAdornment}
Expand Down
12 changes: 12 additions & 0 deletions packages/material-ui/src/InputBase/InputBase.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,18 @@ describe('<InputBase />', () => {
assert.strictEqual(input.props().required, true);
});
});

describe('focused', () => {
it('prioritizes context focus', () => {
wrapper.setState({ focused: true });

setFormControlContext({ focused: false });
assert.strictEqual(wrapper.hasClass(classes.focused), false);

setFormControlContext({ focused: true });
assert.strictEqual(wrapper.hasClass(classes.focused), true);
});
});
});

describe('componentDidMount', () => {
Expand Down