Skip to content

fix(React): Use componentDidMount instead of componentWillMount#321

Merged
azu merged 2 commits intoalmin:masterfrom
koba04:deprecate-cwm
Jan 19, 2018
Merged

fix(React): Use componentDidMount instead of componentWillMount#321
azu merged 2 commits intoalmin:masterfrom
koba04:deprecate-cwm

Conversation

@koba04
Copy link
Copy Markdown
Contributor

@koba04 koba04 commented Jan 19, 2018

React is going to deprecate componentWillMount so Almin should use componentDidMount instead of componentWillMount.
This PR includes fixes for documentation and an example and a source of almin-react-container.

Should I separate this PR each one?

@azu
Copy link
Copy Markdown
Member

azu commented Jan 19, 2018

Should I separate this PR each one?

Almin project use Conventional Commits. (We want to improve CONTRIBUTING Guide too)

If the scope is difference, you should separate commits.
In this case, separate commits into counter and almin-react-container.
(The scope is often package name)

FYI: my other project describe this context in detail. https://github.com/textlint/textlint/blob/master/docs/CONTRIBUTING.md#git-commit-message-format

Can you separate the commit into follows?

  • fix(counter): Use componentDidMount instead of componentWillMount
  • fix(almin-react-container): Use componentDidMount instead of componentWillMount

Actually, lerna use this commit message for generating CHANGELOG and versioning.

@azu
Copy link
Copy Markdown
Member

azu commented Jan 19, 2018

Should I separate this PR each one?

A single PR includes multiple commits is OK.

@azu
Copy link
Copy Markdown
Member

azu commented Jan 19, 2018

FYI: I've updated commit message style document #322

@koba04
Copy link
Copy Markdown
Contributor Author

koba04 commented Jan 19, 2018

@azu Thank you for your help! I've fixed it.

Copy link
Copy Markdown
Member

@azu azu left a comment

Choose a reason for hiding this comment

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

LGTM.

@azu azu merged commit 98e915a into almin:master Jan 19, 2018
@azu
Copy link
Copy Markdown
Member

azu commented Jan 19, 2018

Thanks!

@koba04
Copy link
Copy Markdown
Contributor Author

koba04 commented Jan 19, 2018

Thank you!

@koba04 koba04 deleted the deprecate-cwm branch January 19, 2018 15:30
azu added a commit that referenced this pull request Feb 24, 2018
We have used componentDidMount instead of componentWillMount in #321
But, Almin React Container should be initialized before other component.
Because, this component subscribe `Context#onChange`.
In other words, Almin React Container can not handle `Context#onchange` when some store has been changed in Other Component#componentDidMount.
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.

2 participants