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.

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

componentWillReceiveProps -> componentDidUpdate #635

Merged
merged 2 commits into from
Jan 8, 2020
Merged

componentWillReceiveProps -> componentDidUpdate #635

merged 2 commits into from
Jan 8, 2020

Conversation

mfix22
Copy link
Contributor

@mfix22 mfix22 commented Aug 19, 2019

Instead of using componentDidUpdate, you could also update this component to use getDerivedStateFromProps if you are willing to put this.input in state instead:

static getDerivedStateFromProps(props, state) {
  if (props.value !== state.value) {
    if (state.input === document.activeElement) {
      return { blurValue: String(props.value).toUpperCase() }
    } else {
      return { value: String(props.value).toUpperCase(), blurValue: !state.blurValue && String(props.value).toUpperCase() }
    }
  }
}

...

ref={ input => this.setState({ input }) }

Let me know if you all prefer one over the other.

Begin #634

@mikkpokk
Copy link

mikkpokk commented Nov 11, 2019

@mfix22 Here is actually bugless solution using componentDidUpdate method.

export class EditableInput extends (PureComponent || Component) {
    input = React.createRef();

    ...

    componentDidUpdate(prevProps, prevState) {
        const input = this.input.current || null;

        if (
            this.props.value !== this.state.value && 
            (prevProps.value !== this.props.value || prevState.value !== this.state.value)
        ) {
            if (input === document.activeElement) {
                this.setState({
                    blurValue: String(this.props.value).toUpperCase()
                });
            } else {
                this.setState({
                    value: String(this.props.value).toUpperCase(),
                    blurValue: !this.state.blurValue && String(this.props.value).toUpperCase()
                });
            }
        }
    }

    ...

    render() {
        ...
        <input
                    ref={this.input}
                    ....
            />

        ...
    }

@dreyks
Copy link

dreyks commented Dec 1, 2019

any updates on this one?

@rinick
Copy link

rinick commented Dec 5, 2019

@mfix22 there are 3 places that use componentWillReceiveProps, but this PR only fixed one of them

@mfix22
Copy link
Contributor Author

mfix22 commented Dec 6, 2019

@rinick is that preventing this PR from being merged? I can probably take a look at the other ones this weekend. I don't think it is a problem for them to come from separate PRs or even separate people.

@mfix22
Copy link
Contributor Author

mfix22 commented Dec 8, 2019

@casesandberg anything you need from me before merging this? Do you want to go the cDU route, or the gDSFP route?

@rinick
Copy link

rinick commented Dec 9, 2019

@rinick is that preventing this PR from being merged? I can probably take a look at the other ones this weekend. I don't think it is a problem for them to come from separate PRs or even separate people.

I'm not a maintainer of this this project, I just feel doing it right would increase the chance of it getting merged.

These 3 places are basically the same issue. It would be really weird if it's fixed by 3 different people with 3 different PRs

@mfix22
Copy link
Contributor Author

mfix22 commented Dec 11, 2019

I just pushed the other 2 PRs. The maintainer can decide which to merge in where. 🤷‍♂

@casesandberg casesandberg merged commit 597add3 into casesandberg:master Jan 8, 2020
@casesandberg
Copy link
Owner

Looks great. This will be released shortly as [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants