Skip to content

Conversation

@ElreyB
Copy link
Contributor

@ElreyB ElreyB commented Sep 18, 2018

Fixes # (issue) #540

Type of Change

Please delete options that are not relevant.

  • Remove deprecated componentWillReceiveProps with getDerivedStateFromProps
  • Remove deprecated componentWillMount with componentDidMount

How Has This Been Tested? (using unit testing)

Checklist: (Feel free to delete this section upon completion)

  • My code follows the style guidelines of this project (I have run yarn prettier-fix && yarn lint)
  • New and existing unit tests pass locally with my changes (I have run yarn test)
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@ebrillhart
Copy link
Contributor

This actually should, ideally, not be a breaking change in the sense that it should not affect the functionality for any users of the library, only how the code is written and runs under the hood!

@ElreyB
Copy link
Contributor Author

ElreyB commented Sep 19, 2018

@ebrillhart cool beans. I think I just pick the wrong option for that section.

Copy link
Contributor

@kale-stew kale-stew left a comment

Choose a reason for hiding this comment

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

This is looking really great! 🎉 I've left a couple of comments and questions 🙂

transformStyle: 'flat'
});

function buildSlideReference(props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could I ask why you opted to move this method outside the component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kale-stew getDerviedStateFromProps does not have access to the component instance. Since buildSlideReference didn't have any component instances in the method itself, I opted to take it out of the class and use it as a regular method that the class can use.

@ElreyB ElreyB force-pushed the chore/remove-deprecated-lifecycles branch from ceeba84 to 56a907b Compare September 24, 2018 15:21
Copy link
Contributor

@parkerziegler parkerziegler left a comment

Choose a reason for hiding this comment

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

On first glance this looks good @ElreyB! See my one comment about the componentDidUpdate signature. Once we get that up, I'll pull locally and test.

@ElreyB ElreyB force-pushed the chore/remove-deprecated-lifecycles branch from 331ee3b to 99407c3 Compare September 24, 2018 17:21
@parkerziegler
Copy link
Contributor

@ebrillhart @kale-stew I symlinked this locally against one of my Spectacle presentations and it worked as expected. I also ran the demo locally after fresh install and all seemed normal there. I think we are likely good for a merge.

It may be worth noting in the CHANGELOG that users will need to ensure they have React and ReactDOM ^16.3.1 as their local dependency (earliest version with stable getDerivedStateFromProps). Not a huge issue, people should hopefully get that update automatically if they yarn upgrade / npm update or start fresh from the boilerplate, but I can see folks just upgrading their spectacle dep, running yarn / npm i, and not getting the React / ReactDOM update b/c of yarn.lock / package-lock.json.

@ebrillhart
Copy link
Contributor

I will merge this and make a note of dependency stuff in the release notes for next release!

@ebrillhart ebrillhart merged commit 986c299 into master Sep 25, 2018
@ebrillhart ebrillhart deleted the chore/remove-deprecated-lifecycles branch September 25, 2018 17:50
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