[InputBase] Convert to function component #15446

adeelibr · 2019-04-22T15:13:53Z

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

Related to #15231

mui-pr-bot · 2019-04-22T15:25:04Z

@material-ui/core: parsed: -0.27% 😍, gzip: -0.25% 😍

Details of bundle changes.

Comparing: b20013f...6c8e958

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.27%	-0.25%	311,422	310,592	84,611	84,396
@material-ui/core/Paper	0.00%	+0.01% 🔺	67,623	67,623	20,124	20,126
@material-ui/core/Paper.esm	0.00%	0.00%	60,988	60,988	19,020	19,020
@material-ui/core/Popper	0.00%	+0.02% 🔺	31,114	31,114	10,803	10,805
@material-ui/core/Textarea	0.00%	0.00%	5,472	5,472	2,368	2,368
@material-ui/core/TrapFocus	0.00%	-0.06%	3,731	3,731	1,565	1,564
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	15,943	15,943	5,777	5,777
@material-ui/core/useMediaQuery	0.00%	0.00%	2,106	2,106	974	974
@material-ui/lab	0.00%	-0.01%	141,001	141,001	42,656	42,653
@material-ui/styles	0.00%	+0.01% 🔺	51,151	51,151	15,148	15,150
@material-ui/system	0.00%	+0.03% 🔺	11,765	11,765	3,923	3,924
Button	0.00%	+0.02% 🔺	85,905	85,905	25,734	25,739
Modal	0.00%	+0.03% 🔺	20,579	20,579	6,604	6,606
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	51,154	51,154	11,250	11,250
docs.main	-0.13%	-0.07%	648,743	647,882	202,416	202,278
packages/material-ui/build/umd/material-ui.production.min.js	-0.29%	-0.22%	293,137	292,283	82,531	82,346

Generated by 🚫 dangerJS against 6c8e958

adeelibr · 2019-04-22T20:31:15Z

I am having an issue setting the initial focus of the input field, somehow the label overlaps with the input fields

I know it is something to do with something like

-  import { setRef } from '../utils/reactHelpers';
+ import { useForkRef } from '../utils/reactHelpers';

Maybe I am wrong here, I am not sure. 🙍‍♂️

Also I tried replacing withStyles with makeStyles

+ const useStyles = makeStyles(theme => {}, { name: 'MuiInputBase' });
- export const styles = theme => {}

- export default withStyles(styles, { name: 'MuiInputBase' })(withFormControlContext(InputBase));
+ export default withFormControlContext(InputBase);

But using makeStyles messes up the styling for the components 🤔 💭 Can you kindly help me here.

Can you please point me in the right direction, @joshwooding

joshwooding

Nice start! Looks like you forgot to use .current when accessing the stored ref,

We use instance to refer to the actual instance (context: #15347 (comment))

The next step would be to swap the setRef implementation for the useForkRef implementation. 👍

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

adeelibr · 2019-04-23T13:37:37Z

using inputRef.current solved the focus issue for me :) thanks. It looks something like this now

Sorry for asking again 😅@joshwooding

In order for me to replace setRef with useForkRef should the implementation be something like this.

- const InputBase = React.forwardRef(function InputBase(props, ref) {
+ const InputBase = props => {

  // can be removed once we drop support for non ref forwarding class components
  const handleOwnRef = React.useCallback(ref => {
    inputRef.current = ReactDOM.findDOMNode(ref);
  }, []);
  const handleRef = useForkRef(innerRef, handleOwnRef);

const handleRefInput = instance => {
    const { inputProps } = props;
    inputRef.current = instance;

    warning(
      !instance || instance instanceof HTMLInputElement || instance.focus,
      [
        'Material-UI: you have provided a `inputComponent` to the input component',
        'that does not correctly handle the `inputRef` property.',
        'Make sure the `inputRef` property is called with a HTMLInputElement.',
      ].join('\n'),
    );

-    let refProp;

-    if (inputRefProp) {
-      refProp = inputRefProp;
-    } else if (inputProps && inputProps.ref) {
-      refProp = inputProps.ref;
-    }

-    setRef(refProp, instance);
  };

+ return (
-    <div className={className} onClick={handleClick} ref={innerRef} {...other}>
_    <div className={className} onClick={handleClick} ref={handleRef} {...other}>

Or am I doing it absolutely wrong

adeelibr · 2019-04-23T17:01:46Z

I don't know if this is the right approach to it or not but is this how it was suppose to be

  function onGetRef() {
    let refProp;
    if (inputRefProp) {
      refProp = inputRefProp;
    } else if (props.inputProps && props.inputProps.ref) {
      refProp = props.inputProps.ref;
    }
    return refProp;
  }
  const handleOwnRef = onGetRef()
  const handleRefIntermediary = useForkRef(inputRef, handleOwnRef);
  const handleRef = useForkRef(ref, handleRefIntermediary);

const handleRefInput = instance => {
-    const { inputProps } = props;
    inputRef.current = instance;

    warning(
      !instance || instance instanceof HTMLInputElement || instance.focus,
      [
        'Material-UI: you have provided a `inputComponent` to the input component',
        'that does not correctly handle the `inputRef` property.',
        'Make sure the `inputRef` property is called with a HTMLInputElement.',
      ].join('\n'),
    );

-    let refProp;

-    if (inputRefProp) {
-      refProp = inputRefProp;
-    } else if (inputProps && inputProps.ref) {
-      refProp = inputProps.ref;
-    }

-    setRef(refProp, instance);
  };

Am I even looking in the right direction here? Can you kindly just point me to the right direction 😅

joshwooding · 2019-04-23T20:51:50Z

I don't know if this is the right approach to it or not but is this how it was suppose to be
Am I even looking in the right direction here? Can you kindly just point me to the right direction 😅

I would do:

const handleInputRefWarning = instance => {
    warning(
      !instance || instance instanceof HTMLInputElement || instance.focus,
      [
        'Material-UI: you have provided a `inputComponent` to the input component',
        'that does not correctly handle the `inputRef` property.',
        'Make sure the `inputRef` property is called with a HTMLInputElement.',
      ].join('\n'),
    );
  };
  const handleInputPropsRefProp = useForkRef(inputPropsProp.ref, handleInputRefWarning);
  const handleInputRefProp = useForkRef(inputRefProp, handleInputPropsRefProp);
  const handleRef = useForkRef(inputRef, handleInputRefProp);

I don't see any benefit preferring inputRefProp over inputPropsProp.ref so we should probably just fork both.

joshwooding

Just a bit more polishing and then on to tests 👍

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

adeelibr · 2019-04-23T21:24:56Z

@joshwooding Can you kindly review again. I will update the API docs. Once you give me the go ahead. 😄 & then move on to the tests.

joshwooding

Looks good so far!

Stale

adeelibr · 2019-04-23T22:30:36Z

I am running into an issue where when I ran the script yarn docs:api on a windows machine. It converts all my files into LF type. Which then shows all the files in pages/**.** as modified.

I have tried doing all 3 options

git config --global core.autocrlf true
git config --global core.autocrlf false
git config --global core.autocrlf input

I get the same result ..

This is the same issue as #12584

I can try running yarn docs:api command early when I get to office via a mac for now.

Details on OS:
OS: Windows
Git version: 2.21.0.windows.1

joshwooding · 2019-04-23T22:36:44Z

@adeelibr That happens to me too, when I git add -A it sorts the line endings out though.

adeelibr · 2019-04-23T22:57:44Z

Yes you are absolutely right 🎉 Since innerRef prop type already had the @ignore attribute. It was never part of the docs. So no changes to add there.

Thank you for the help @joshwooding 🙇

Should I move on to the tests now? 😅

joshwooding · 2019-04-23T23:06:27Z

Thank you for the help @joshwooding 🙇

Should I move on to the tests now? 😅

No worries. Yep, tests now :P They will fail due to functional components not having instances. We try to test our components using their public API i.e. props so anything that tests internals should be changed (refs and state) to do so and we are moving away from shallow rendering so if you have a change it would be nice if you could change that too.

adeelibr · 2019-04-24T21:52:25Z

I have been trying to wrap my head around using testing for hooks & I have been struggling with all day. I saw these files to get some context on how I could test a react hook based component

But I think I got confused more 😿 I just wanted to verify that the approach I am doing is the right way to do it or not, can you kindly help (point me in some direction) @joshwooding

Before

it('should reset the focused state', () => {
      const handleBlur = spy();
      const wrapper = mount(<InputBase muiFormControl={{ onBlur: handleBlur }} />);
      // We simulate a focused input that is getting disabled.
      setState(wrapper, {
        focused: true,
      });
      wrapper.setProps({
        disabled: true,
      });
      assert.strictEqual(wrapper.find('InputBase').instance().state.focused, false);
      assert.strictEqual(handleBlur.callCount, 1);
});

After

it('should reset the focused state', () => {
      const handleBlur = spy();
      const wrapper = mount(<InputBase muiFormControl={{ onBlur: handleBlur }} />);
      
      // We simulate a focused input that is getting disabled.'
      const inputEl = wrapper.find('input');
      inputEl.simulate('focus');
      wrapper.setProps({ disabled: true });

      assert.strictEqual(inputEl.is(':focus'), false);
      assert.strictEqual(handleBlur.callCount, 1);
});

joshwooding · 2019-04-24T21:58:53Z

@adeelibr Looks good to me, I would add an assert that after you simulate focus that the input is actually focused.

adeelibr · 2019-04-24T22:08:58Z

@adeelibr Looks good to me, I would add an assert that after you simulate focus that the input is actually focused.

Okay, so on doing

const inputEl = wrapper.find('input');
inputEl.simulate('focus');
assert.strictEqual(inputEl.is(':focus'), true);

inputEl.is(':focus') is always false 😅

Back to square one, 🖼 Let me try/finding another approach. 😨

To add more on it, document.activeElement shows no active element. When I do inputEl.simulate('focus'); but there is a <input />. I verified it using console.log('inputEl', wrapper.find('input').debug())

Can you kindly tell as to what am I doing wrong here ..

eps1lon · 2019-04-25T07:46:06Z

Everything related to setState or mocking of instance variables was only an escape hatch. You should try to test how this component would be used. For library components this should be done with setProps 90% of the time.

assert.strictEqual(inputEl.is(':focus'), true); inputEl is a misnomer. This is the return value from wrapper.find which is a ReactWrapper that takes an EnzymeSelector in .is. EnzymeSelectors do not support css pseudo classes as far as I can tell: https://airbnb.io/enzyme/docs/api/selector.html

adeelibr · 2019-04-25T08:19:33Z

EnzymeSelectors do not support css pseudo classes

I thought this PR solved that issue for enzyme enzymejs/enzyme#1965

Thank you for pointing that out, I was scratching my head all day for this.

Everything related to setState or mocking of instance variables was only an escape hatch. You should try to test how this component would be used. For library components this should be done with setProps 90% of the time.

On it. 😄

adeelibr · 2019-04-25T20:12:19Z

I am sorry but testing hooks implementation with enzyme is so difficult. I wish we could use react-testing-library! I have been struggling with this for 2 days & I haven't made any progress, I am so sorry. 😭

joshwooding · 2019-04-25T21:02:47Z

I am sorry but testing hooks implementation with enzyme is so difficult. I wish we could use react-testing-library! I have been struggling with this for 2 days & I haven't made a any progress, I am so sorry. 😭

No worries, I've fixed a couple of tests for you.

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

adeelibr · 2019-04-25T21:44:30Z

No worries, I've fixed a couple of tests for you.

Thank you, there was a lot for me learn here. 🥇 💯 😄

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

eps1lon · 2019-04-27T08:13:55Z

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

-      assert.strictEqual(handleFilled.callCount, 2);
+      const { wrapper } = setup();
+      wrapper.find('input').instance().value = 'stub';
+      assert.strictEqual(wrapper.find('input').instance().value, 'stub');


This is asserting what you did in the first line. Could you just dispatch a input change event?

I’m not sure what benefit this test has realistically. Testing that checkDirty is called when uncontrolled and isn't called when controlled seems like a better test

To be honest I don't even know why we checkDirty only once when uncontrolled. Not sure what the use case is. Maybe a better test could help here.

I’m not sure why either.

Edit: I know now :P

For the test 'should check that the component is uncontrolled' would it be okay if i did something like this would that be okay 💭

1- Pass in a default value to the component
2- Then use an onChange (spy) method for trying to change the input (which won't work)
3- Assert that default value remains intact & default value is the same as the input.instance().value

(absolutely not sure about this approach =P) 😅

I have this test

it('should check that the component is uncontrolled', () => { const { wrapper } = setup(); wrapper.setProps({ defaultValue: 'stub' }); const handleChange = spy(); wrapper.setProps({ onChange: handleChange }); wrapper.update(); const event = { target: { value: 'foo' } }; wrapper.find('input').simulate('change', event); assert.strictEqual(handleChange.callCount, 1); const inputEl = wrapper.find('input'); console.log(inputEl.debug()); console.log(inputEl.props()); console.log(inputEl.html()); assert.strictEqual(wrapper.find('input').instance().value, 'stub'); // THIS FAILS 😞 });

Response for console.log(inputEl.debug());

<input aria-invalid={[undefined]} aria-describedby={[undefined]} autoComplete={[undefined]} autoFocus={[undefined]} className="" + defaultValue="stub" disabled={[undefined]} id={[undefined]} name={[undefined]} onBlur={[Function: handleBlur]} onChange={[Function: handleChange]} onFocus={[Func tion: handleFocus]} onKeyDown={[undefined]} onKeyUp={[undefined]} placeholder={[undefined]} readOnly={[undefined]} required={[undefined]} rows={[undefined]} value={[undefined]} type="text" />

Response for console.log(inputEl.props())

{ 'aria-invalid': undefined, 'aria-describedby': undefined, autoComplete: undefined, autoFocus: undefined, className: '', defaultValue: 'stub', disabled: undefined, id: undefined, name: undefined, onBlur: [Function: handleBlur], onChange: [Function: handleChange], onFocus: [Function: handleFocus], onKeyDown: undefined, onKeyUp: undefined, placeholder: undefined, readOnly: undefined, required: undefined, rows: undefined, value: undefined, type: 'text' }

But the response for 'console.log(inputEl.html());` is 😭 😥

<input class="" type="text" value="hell">

Is my understanding of the testing wrong, shouldn't this be true assert.strictEqual(wrapper.find('input').instance().value, 'stub'); but it is false

For some reason your defaultValue isn’t being set through setProps. I can have a look later but I don’t think you need wrapper.update there?

This won't really test a lot since the value is set outside the InputBase and not in onChange. Your test would only check that defaultValue sets value and onChange is called. My guess why the current test doesn't work is due to jsdom and it's weird input value updating.

I am sorry, but I can't think of a way to test this case... 😞 😞

I've removed the check state tests. I don't think they provide any value, all we care is that the component works in an uncontrolled and controlled state, which the other tests already test.

cc @eps1lon @oliviertassinari

oliviertassinari · 2019-05-11T14:58:58Z

Rebasing on the next branch […]

oliviertassinari

I have tried to push the changes one step further. I might have done too ambitious changes, let me know by reviewing the changes.

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

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

adeelibr marked this pull request as ready for review April 22, 2019 20:31

joshwooding added component: InputBase new feature New feature or request labels Apr 22, 2019

joshwooding suggested changes Apr 22, 2019

View reviewed changes

joshwooding 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 Apr 22, 2019

joshwooding previously requested changes Apr 23, 2019

View reviewed changes

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

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

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

joshwooding mentioned this pull request Apr 23, 2019

Migration to hooks #15231

Closed

29 tasks

joshwooding reviewed Apr 23, 2019

View reviewed changes

joshwooding 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 Apr 23, 2019

joshwooding reviewed Apr 25, 2019

View reviewed changes

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

eps1lon previously requested changes Apr 27, 2019

View reviewed changes

oliviertassinari self-assigned this May 11, 2019

adeelibr and others added 3 commits May 11, 2019 17:12

Initial commit

cc6bfc8

remove 'should check if' tests

d4c0ea8

test remove unwrap

921332f

oliviertassinari force-pushed the inputbase-hooks-convert branch 5 times, most recently from 3c34a8c to 9aad0c4 Compare May 11, 2019 15:21

remove withFormControlContext

4a33463

oliviertassinari force-pushed the inputbase-hooks-convert branch from 9aad0c4 to 4a33463 Compare May 11, 2019 15:22

oliviertassinari requested review from joshwooding and eps1lon and removed request for joshwooding and eps1lon May 11, 2019 15:22

oliviertassinari reviewed May 11, 2019

View reviewed changes

joshwooding reviewed May 11, 2019

View reviewed changes

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

joshwooding reviewed May 11, 2019

View reviewed changes

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

oliviertassinari removed their assignment May 11, 2019

Remove muiFormControlRef

b3de90e

oliviertassinari reviewed May 11, 2019

View reviewed changes

packages/material-ui/src/InputBase/InputBase.test.js Show resolved Hide resolved

joshwooding reviewed May 11, 2019

View reviewed changes

packages/material-ui/src/InputBase/InputBase.test.js Show resolved Hide resolved

joshwooding approved these changes May 11, 2019

View reviewed changes

joshwooding requested a review from eps1lon May 11, 2019 21:55

oliviertassinari added the PR: good for merge label May 14, 2019

oliviertassinari merged commit e802aeb into mui:next May 14, 2019

adeelibr deleted the inputbase-hooks-convert branch May 20, 2019 21:39

oliviertassinari mentioned this pull request Jun 28, 2019

[TextField] onBlur event argument might be undefined #16402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InputBase] Convert to function component #15446

[InputBase] Convert to function component #15446

adeelibr commented Apr 22, 2019 •

edited

Loading

mui-pr-bot commented Apr 22, 2019 •

edited

Loading

adeelibr commented Apr 22, 2019 •

edited

Loading

joshwooding left a comment

adeelibr commented Apr 23, 2019 •

edited

Loading

adeelibr commented Apr 23, 2019

joshwooding commented Apr 23, 2019

joshwooding left a comment

adeelibr commented Apr 23, 2019

joshwooding left a comment

adeelibr commented Apr 23, 2019 •

edited

Loading

joshwooding commented Apr 23, 2019

adeelibr commented Apr 23, 2019

joshwooding commented Apr 23, 2019

adeelibr commented Apr 24, 2019 •

edited

Loading

joshwooding commented Apr 24, 2019

adeelibr commented Apr 24, 2019 •

edited

Loading

eps1lon commented Apr 25, 2019

adeelibr commented Apr 25, 2019

adeelibr commented Apr 25, 2019 •

edited

Loading

joshwooding commented Apr 25, 2019

adeelibr commented Apr 25, 2019

eps1lon Apr 27, 2019

joshwooding Apr 27, 2019

eps1lon Apr 27, 2019

joshwooding Apr 29, 2019 •

edited

Loading

adeelibr Apr 30, 2019

adeelibr May 1, 2019 •

edited

Loading

joshwooding May 2, 2019

joshwooding May 7, 2019

adeelibr May 9, 2019 •

edited

Loading

joshwooding May 10, 2019 •

edited

Loading

oliviertassinari commented May 11, 2019

oliviertassinari left a comment

[InputBase] Convert to function component #15446

[InputBase] Convert to function component #15446

Conversation

adeelibr commented Apr 22, 2019 • edited Loading

mui-pr-bot commented Apr 22, 2019 • edited Loading

adeelibr commented Apr 22, 2019 • edited Loading

joshwooding left a comment

Choose a reason for hiding this comment

adeelibr commented Apr 23, 2019 • edited Loading

adeelibr commented Apr 23, 2019

joshwooding commented Apr 23, 2019

joshwooding left a comment

Choose a reason for hiding this comment

adeelibr commented Apr 23, 2019

joshwooding left a comment

Choose a reason for hiding this comment

adeelibr commented Apr 23, 2019 • edited Loading

joshwooding commented Apr 23, 2019

adeelibr commented Apr 23, 2019

joshwooding commented Apr 23, 2019

adeelibr commented Apr 24, 2019 • edited Loading

joshwooding commented Apr 24, 2019

adeelibr commented Apr 24, 2019 • edited Loading

eps1lon commented Apr 25, 2019

adeelibr commented Apr 25, 2019

adeelibr commented Apr 25, 2019 • edited Loading

joshwooding commented Apr 25, 2019

adeelibr commented Apr 25, 2019

eps1lon Apr 27, 2019

Choose a reason for hiding this comment

joshwooding Apr 27, 2019

Choose a reason for hiding this comment

eps1lon Apr 27, 2019

Choose a reason for hiding this comment

joshwooding Apr 29, 2019 • edited Loading

Choose a reason for hiding this comment

adeelibr Apr 30, 2019

Choose a reason for hiding this comment

adeelibr May 1, 2019 • edited Loading

Choose a reason for hiding this comment

joshwooding May 2, 2019

Choose a reason for hiding this comment

joshwooding May 7, 2019

Choose a reason for hiding this comment

adeelibr May 9, 2019 • edited Loading

Choose a reason for hiding this comment

joshwooding May 10, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented May 11, 2019

oliviertassinari left a comment

Choose a reason for hiding this comment

adeelibr commented Apr 22, 2019 •

edited

Loading

mui-pr-bot commented Apr 22, 2019 •

edited

Loading

adeelibr commented Apr 22, 2019 •

edited

Loading

adeelibr commented Apr 23, 2019 •

edited

Loading

adeelibr commented Apr 23, 2019 •

edited

Loading

adeelibr commented Apr 24, 2019 •

edited

Loading

adeelibr commented Apr 24, 2019 •

edited

Loading

adeelibr commented Apr 25, 2019 •

edited

Loading

joshwooding Apr 29, 2019 •

edited

Loading

adeelibr May 1, 2019 •

edited

Loading

adeelibr May 9, 2019 •

edited

Loading

joshwooding May 10, 2019 •

edited

Loading