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

Removed unnecessary component state, used props instead #20791

Merged
merged 3 commits into from
Jan 22, 2020

Conversation

alexisshriov
Copy link
Contributor

@alexisshriov alexisshriov commented Jan 22, 2020

removed unnecessary component state

Description

there is no need to have a state in this component, we can use props directly

Documentation

this is part of the documentation of LanguageSwitcher, is a code example.

Related Issues

…eFromProps, checked for props changes to trigger updates
@alexisshriov alexisshriov requested a review from a team as a code owner January 22, 2020 20:02
@pieh
Copy link
Contributor

pieh commented Jan 22, 2020

I was looking through this component, and now I wonder if we even need to have state here at all? It just seems like it's just shortcut for this.props.i18n.language (?)

@alexisshriov
Copy link
Contributor Author

@pieh you are probably right, the only place where it's used is in renderLanguageChoice, and is just making a comparison, it could use props for the same purpose, do you want me to change it?

@pieh
Copy link
Contributor

pieh commented Jan 22, 2020

@pieh you are probably right, the only place where it's used is in renderLanguageChoice, and is just making a comparison, it could use props for the same purpose, do you want me to change it?

If it doesn't have any functional difference, we should change it to drop using state.

When to Use Derived State section in react docs is good place to read a little bit about usage of derived state, and I don't think this case is good one for it

@pieh
Copy link
Contributor

pieh commented Jan 22, 2020

It even explitely calls "Unconditionally copying props to state" as anti-pattern ;) few section later

@alexisshriov
Copy link
Contributor Author

I was using it as a replacement for componentWillReceiveProps which was already there, but since we don't even need to have a component state then the whole thing is unnecessary, if you want me I can remove the component state and just use props

@pieh
Copy link
Contributor

pieh commented Jan 22, 2020

if you want me I can remove the component state and just use props

If you could do that, that would be awesome!

@alexisshriov alexisshriov changed the title changed deprecated method componentWillReceiveProps to getDerivedStat… Removed unnecessary component state, used props instead Jan 22, 2020
@alexisshriov
Copy link
Contributor Author

@pieh okay I think it's done

@pieh
Copy link
Contributor

pieh commented Jan 22, 2020

Ok, there is just that line after edited code block:

This is a pretty simple component. We're setting the language state based on the i18n prop so that we can check which language is currently active and show that in our menu.

That needs a bit of adjust as we don't have state anymore - maybe something like (additionally trying to avoid language like "easy"):

This is a pretty small component. We are using i18n prop so that we can check which language is currently active and show that in our menu

(feel free to reword in different way)

@alexisshriov
Copy link
Contributor Author

@pieh you are right!, okay I finished the text change, let me know if you want me to change anyhting else :)

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@pieh pieh added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jan 22, 2020
@pieh
Copy link
Contributor

pieh commented Jan 22, 2020

I think we are good - it should automatically merge pull request now once all the checks finish

@alexisshriov
Copy link
Contributor Author

@pieh thank you!

@gatsbybot gatsbybot merged commit 77abee1 into gatsbyjs:master Jan 22, 2020
@gatsbot
Copy link

gatsbot bot commented Jan 22, 2020

Holy buckets, @alexisshriov — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants