[TextField] Fix focused prop so it triggers focus state

[amireynoso]

WHY are these changes introduced?

Resolves #985

The component accepts a focused prop which should force the field to be focused. However, it is not currently working (take a look at the issue for a reduced test case).

WHAT is this pull request doing?

I based my changes in the <SearchField /> implementation (component and tests), which also accepts a focused prop and it works as expected. This is a breakdown of the changes:

• It now takes the focused prop into consideration when setting the value for state.focus (used to be false by default)
• Forces the input's focus state on componentDidMount() if the prop is true.
• Forces the input's focus state (or blur state) on componentDidUpdate() in case the prop is toggled
• Scoped the tests related to this prop into their own describe block
• The current test claimed to be testing something it was not testing (nor doing): "focuses input and calls onFocus() when focused prop has been updated to true"
  • I refactored this test a little bit to test that the element is in focus when the prop is set to true
• Added two new tests to check the prop can be toggled

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {Page, TextField} from '../src';

interface State {
  value: string;
}

export default class Playground extends React.Component<{}, State> {
  state = {
    value: 'Lorem ipsum',
  };

  handleChange = (value: string) => {
    this.setState({value});
  };

  render() {
    return (
      <Page title="Playground">
        <TextField
          label="Focused text field"
          value={this.state.value}
          onChange={this.handleChange}
          multiline
          focused
        />
      </Page>
    );
  }
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility

[AndrewMusgrave]

The ref should always be available in componentDidMount? We should be able to omit that check. The JS styleguide also prefers early returns so we can probably change this to

if (!this.props.focused) return;
this.input.focus();

[amireynoso]

Sounds good!

[AndrewMusgrave]

Same comment about the early return here as well

[AndrewMusgrave]

Should we blur when the props change? Using the focused prop forces focus on the textfield but omitting the prop doesn't necessarily mean they want the field to be blurred. What are your thoughts on this?

[amireynoso]

In this case, the blur event would only trigger if the focused prop changed values and it wouldn't be triggered on mount if focused is false or not defined.

One use case is if focused is being set programatically based on an interaction with another component. If the focus is removed (programatically), I'd expect it to trigger the blur event.

I saw this on the <SearchField /> component and thought it was a nice addition.

[AndrewMusgrave]

Works for me, how about you @solonaarmstrong ?

[amireynoso]

Hey @AndrewMusgrave, thanks for your review! I addressed the comments, would you mind taking another look?

[amireynoso]

@solonaarmstrong Hey Solona! Would you help me with a review of this PR? I wasn't sure who else to tag, feel free to tag someone else. Thank you!

[AndrewMusgrave]

Sorry I already gave feedback so this doesn't show up on my review list anymore, I'll get to it later today 🙏

[AndrewMusgrave]

Works for me, how about you @solonaarmstrong ?

[AndrewMusgrave]

🎩 💯 One comment about the changelog, looks good other than that. Thanks for fixing this ❤️

[solonaarmstrong-zz]

What's the difference between focused and autoFocus? It looks as though autoFocus is the one that actually places the cursor in the field?

[AndrewMusgrave]

Autofocus will focus the textfield when it renders, focused maintains focus on the textfield