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

Move isRtl from props to Redux for PersonalRecentProjects #19608

Merged
merged 5 commits into from Dec 13, 2017

Conversation

joshlory
Copy link
Contributor

Continue isRtl Redux cleanup.

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Thank you for converting these components into classes!


viewAllProjects() {
this.setState({showAll: true, showApp: ''});
},
}
Copy link
Member

Choose a reason for hiding this comment

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

onSelectApp and viewAllProjects do not appear to be properly bound to this here. I believe the preferred syntax is:

  onSelectApp = appType => {
    this.setState({showAll: false, showApp: appType});
  }

@islemaster has done a bunch of these conversions and may be able to offer a second opinion. Here is one very long explanation, or you can skip to the end: http://reactkungfu.com/2015/07/why-and-how-to-bind-methods-in-your-react-component-classes/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, good catch! I will fix.

@caleybrock
Copy link
Contributor

So does this require a parent of https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/templates/projects/ProjectWidgetWithData.jsx to be connected? Is that currently always true?

@joshlory
Copy link
Contributor Author

It does. All three usages of <ProjectWidgetWithData> are connected: TeacherHomepage.jsx, StudentHomepage.jsx, & StageExtras.js.

@joshlory
Copy link
Contributor Author

@davidsbailey verified fixed (green-red-green) with a shiny new unit test!

@joshlory joshlory merged commit b242aaf into staging Dec 13, 2017
@joshlory joshlory deleted the personal-recent-projects-redux branch December 13, 2017 22:00
@davidsbailey
Copy link
Member

Thanks for adding these tests! LGTM

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.

None yet

3 participants