Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

refactor(profile): migrate profile page to redux #1218

Merged
merged 1 commit into from
Jun 24, 2016

Conversation

alex-sl-eng
Copy link
Member

@alex-sl-eng alex-sl-eng commented Jun 23, 2016

@huangp
Copy link
Collaborator

huangp commented Jun 23, 2016

Reviewed 22 of 22 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


frontend/src/main/web/src/actions/profile.js, line 41 [r1] (raw file):

        const contentType = res.headers.get('Content-Type')
        if (contentType && includes(contentType, 'json')) {
          return res.json().then((json) => {

I thought this will be done automatically by redux-api-middleware? At least I don't need to do this in Zanata Sync (but I always consume and produce json from the server)


frontend/src/main/web/src/containers/UserProfile/ContentStateFilter.jsx, line 76 [r1] (raw file):

  handleFilterChanged,
  ...props
}) => {

Just curious. What will this function syntax do? Will it apply the pureRenderMixin automatically?


Comments from Reviewable

@alex-sl-eng
Copy link
Member Author

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


frontend/src/main/web/src/actions/profile.js, line 41 [r1] (raw file):

Previously, huangp (Patrick Huang) wrote…

I thought this will be done automatically by redux-api-middleware? At least I don't need to do this in Zanata Sync (but I always consume and produce json from the server)

Not sure myself. Been doing that in other api call.

Comments from Reviewable

@alex-sl-eng
Copy link
Member Author

@huangp

Just curious. What will this function syntax do? Will it apply the pureRenderMixin automatically?

It used for stateless component. And for redux, which is single state, there's no need of pureRenderMixin

@huangp
Copy link
Collaborator

huangp commented Jun 23, 2016

I think they are two different concepts. pureRenderMixin will skip re-render if props don't change (it basically implements a shouldComponentUpdate() method). stateless component is a candidate for it. So now this will re-render each time the parent component re-renders. I don't think it will kill the performance but it's nice to have.
see facebook/react#5677


Review status: 20 of 22 files reviewed at latest revision, 2 unresolved discussions.


a discussion (no related file):
:lgtm: -(Awaiting review) +Reviewed +(On QA)


Comments from Reviewable

@alex-sl-eng
Copy link
Member Author

@huangp You're right. But just found out

Unfortunately ES6 launched without any mixin support. Therefore, there is no support for mixins when you use React with ES6 classes. Instead, we're working on making it easier to support such use cases without resorting to mixins.

But in this case, there shouldn't be any performance issue as the there's only minimal state changes in profile page.

@djansen-redhat
Copy link
Contributor

✅ tested

@alex-sl-eng alex-sl-eng merged commit 5b1e799 into master Jun 24, 2016
@alex-sl-eng alex-sl-eng deleted the user-profile-redux branch June 24, 2016 04:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants