Skip to content

chore(all): Remove bindMethods in favor of Class properties#313

Merged
jeff-phillips-18 merged 1 commit intopatternfly:masterfrom
chalettu:bindMethods-cleanup
Apr 23, 2018
Merged

chore(all): Remove bindMethods in favor of Class properties#313
jeff-phillips-18 merged 1 commit intopatternfly:masterfrom
chalettu:bindMethods-cleanup

Conversation

@chalettu
Copy link
Contributor

@chalettu chalettu commented Apr 18, 2018

fixes #294

@chalettu chalettu force-pushed the bindMethods-cleanup branch from 1f6ccc5 to 1a469d3 Compare April 18, 2018 20:51
@coveralls
Copy link

coveralls commented Apr 18, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 1236

  • 2 of 3 (66.67%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 76.458%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/common/controlled.js 0 1 0.0%
Totals Coverage Status
Change from base Build 1233: -0.2%
Covered Lines: 1669
Relevant Lines: 1995
💛 - Coveralls

jeff-phillips-18
jeff-phillips-18 requested changes Apr 19, 2018
View reviewed changes
Copy link
Member

@jeff-phillips-18 jeff-phillips-18 left a comment

Choose a reason for hiding this comment

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

Let's deprecate the bindMethods function and console.warn when it's used. That way we can remove it in the next major release.

@priley86
Copy link
Member

priley86 commented Apr 19, 2018

yea - we are using those downstream too (directly from pf-react)...so will just make a note for the next major. 👍

@chalettu chalettu force-pushed the bindMethods-cleanup branch 2 times, most recently from be25f7c to 42ca6f1 Compare April 19, 2018 16:08
@chalettu
Copy link
Contributor Author

chalettu commented Apr 19, 2018

Updated PR with a console.warn message reflecting that bindMethods is now being deprecated.

jeff-phillips-18
jeff-phillips-18 reviewed Apr 19, 2018
View reviewed changes
src/common/helpers.js Outdated
console.warn(`
bindMethods usage is being deprecated in favor of class methods.
bindMethods will be deprecated in next major release
`);
Copy link
Member

@jeff-phillips-18 jeff-phillips-18 Apr 19, 2018

Choose a reason for hiding this comment

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

Slight text update:
is deprecated
will be removed

@chalettu chalettu force-pushed the bindMethods-cleanup branch from 42ca6f1 to 022ef25 Compare April 19, 2018 17:31
@chalettu
Copy link
Contributor Author

chalettu commented Apr 19, 2018

@jeff-phillips-18 , I updated the language in the console message. Let me know if you think we need any more changes. Thanks in advance

@priley86
Copy link
Member

priley86 commented Apr 19, 2018 •
edited
Loading

@chalettu did you rebase the inline edit table? Thanks again for tackling this one!

@chalettu chalettu force-pushed the bindMethods-cleanup branch from 022ef25 to b1b007b Compare April 19, 2018 20:17
@chalettu
Copy link
Contributor Author

chalettu commented Apr 19, 2018

@priley86 , I rebased just now so it should be up to date. Let me know if you have any more questions

@priley86
Copy link
Member

priley86 commented Apr 20, 2018

Thanks a lot! I will do a quick sanity check in the morning but this looks good ;)

@jeff-phillips-18
Copy link
Member

jeff-phillips-18 commented Apr 20, 2018

I believe there is still a call to bindMethods in src/components/Wizard/Stories/WizardPatternExample.js

@priley86
Copy link
Member

priley86 commented Apr 20, 2018 •
edited
Loading

ok i manually went through all of the storybooks earlier and couldn't find any issues. I'm good to go here once the WizardPatternExample and WizardExample are updated.

can you please update/rebase the commits to a single commit or just make the first commit a fix()... i.e. fix(all): Remove bindMethods in favor of Class properties ? This way we get those downstream I guess...(would like to validate they still work the same way).

@chalettu chalettu force-pushed the bindMethods-cleanup branch 2 times, most recently from a5ebd43 to 332a20a Compare April 20, 2018 20:19
@chalettu chalettu force-pushed the bindMethods-cleanup branch from 332a20a to 806f8bc Compare April 20, 2018 20:37
@chalettu
Copy link
Contributor Author

chalettu commented Apr 20, 2018

Hi @priley86 , I went ahead and took your advice and collapsed everything into one commit and also took care of the wizard that still had bindMethods. Let me know if you have any questions

@jeff-phillips-18 jeff-phillips-18 merged commit 0404790 into patternfly:master Apr 23, 2018
@chalettu chalettu deleted the bindMethods-cleanup branch April 23, 2018 16:37
@priley86 priley86 mentioned this pull request May 25, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeff-phillips-18 jeff-phillips-18 jeff-phillips-18 approved these changes

+1 more reviewer

@priley86 priley86 priley86 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove bindMethods in favor of Class properties

4 participants

@chalettu @coveralls @priley86 @jeff-phillips-18