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

docs(ComponentExample): replace deprecated lifecycle methods #3533

Merged
merged 3 commits into from
Mar 26, 2019
Merged

docs(ComponentExample): replace deprecated lifecycle methods #3533

merged 3 commits into from
Mar 26, 2019

Conversation

GreenTeaCake
Copy link
Contributor

Relates to #2732: Removes componentWillMount and componentWillReceiveProps from docs/src/components/ComponentDoc/ComponentExample/ComponentExample.js.

@layershifter
Copy link
Member

Wow, that's great ❤️

@codecov-io
Copy link

codecov-io commented Mar 25, 2019

Codecov Report

Merging #3533 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3533   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files         174      174           
  Lines        2725     2725           
=======================================
  Hits         2720     2720           
  Misses          5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb55ca5...201bf72. Read the comment docs.

location: { hash },
} = nextProps

const anchorName = examplePathToHash(examplePath)
Copy link
Member

Choose a reason for hiding this comment

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

We can move anchorName to constructor() as we need it only once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do that only in case if examplePath property is never changed. Current usage of ComponentExample implies such usage. But it is a little bit confusing from general perspective.

Copy link
Member

Choose a reason for hiding this comment

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

Based on nature of ComponentExample it's impossible in production and almost impossible in development, so actually we can do this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I got it. Another reason to have anchorName in state is the ability to use it in static context during derived state computation. If the point for having it as a class field is to perform a computation just once I can do a conditional assignment to the non-empty anchorName.

constructor(props) {
super(props)

const state = ComponentExample.getDerivedStateFromProps(props)
Copy link
Member

Choose a reason for hiding this comment

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

getDerivedStateFromProps() will be dispatcher right after constructor(), we don't need this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use computation for state here to be able to enable source code displaying and set initial value for source code (see an assignment below).

Copy link
Member

Choose a reason for hiding this comment

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

I understand what you mean, but I prefer separation of concerns there: if we will change something in getDerivedStateFromProps() it will break constructor.

Other confusion is placed around lifecycle: https://code.likeagirl.io/understanding-react-component-life-cycle-49bf4b8674de (we call constructor(), getDerivedStateFromProps() and then again getDerivedStateFromProps())

const originalSourceCode = props.exampleSources[props.examplePath]

this.state = {
  anchorName: examplePathToHash(props.examplePath),
  originalSourceCode,
  showCode: anchorName === props.location.hash,
  sourceCode: originalSourceCode,
}

Copy link
Contributor Author

@GreenTeaCake GreenTeaCake Mar 25, 2019

Choose a reason for hiding this comment

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

I can just remove line n93 (...state,).

@layershifter
Copy link
Member

@grumblerchester I've pushed some changes and merged master, can you please check them? 🙄

@GreenTeaCake
Copy link
Contributor Author

@layershifter Hey! I've checked the code locally. Everything works fine

@layershifter layershifter merged commit ffa41bf into Semantic-Org:master Mar 26, 2019
@GreenTeaCake GreenTeaCake deleted the remove-deprecated-lifecycle-methods-from-component-example branch March 26, 2019 20:37
mbakiev pushed a commit to mbakiev/Semantic-UI-React that referenced this pull request Jun 17, 2019
…c-Org#3533)

* docs(ComponentExample): replace deprecated lifecycle methods

* updates

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

Successfully merging this pull request may close these issues.

4 participants