Skip to content

Conversation

@jacky-zhangjb
Copy link
Contributor

No description provided.

@maxceem maxceem self-requested a review September 24, 2019 05:20
@maxceem
Copy link
Collaborator

maxceem commented Sep 24, 2019

@jankyzhang there is some issue with the Member Service. It's not connected with your PR, but I cannot test it properly. So I'm waiting for the member service to come back live.

@maxceem maxceem removed the on hold label Sep 25, 2019
Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Finally could review your PR @jankyzhang.

Generally works good. There are few moments to improve.

  1. Tooltips look broken. See tooltip on Assets Library page:

    image

    And this is how tooltips look like in other places, including Project Listing page:

    image

  2. You did some extra work to implement a new action to load information about user members. Though it also bring some disadvantage. We should use existent action loadMembers as we use it on the project listing page and we use it in many other places when we load user information. This is better because we don't have to re-load information for the same users several times as action loadMembers stores all loaded users in one place.

@jacky-zhangjb
Copy link
Contributor Author

@maxceem
Sorry for the delay reply.
I miss the E-mail notification.
I check it right now.

@jacky-zhangjb
Copy link
Contributor Author

@maxceem,
I have fix two issues now.
Please check the code.

About the issue 2:
In the 'AssetsInfoContainer.jsx' . the 'props.projectMembers' is accroding of the state 'members.members' ,which get from the existent action 'loadMembers'.
If we use the action 'loadMembers' again for 'assetsMembers'.
The 'props.projectMembers' may be changed.
So I implement a new action to load information about user members at first.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thanks for the update @jankyzhang. Both issues are fixed now. Just noticed one performance issue. Please, have a look on the comment below.

@jacky-zhangjb
Copy link
Contributor Author

@maxceem ,
Thanks for the reply.
Now the Service is fail.
I will check and fix the issue today later.
screenshot-connect topcoder-dev com-2019 09 29-22_52_06

@jacky-zhangjb
Copy link
Contributor Author

@maxceem
screenshot-local topcoder-dev com_3000-2019 09 30-01_09_35
Website try to get the user-info about '305384'.
However, just a blank content return.
So the code request to the server again and again, due to the missUserId '305384'.

How we should do about this ?

@maxceem
Copy link
Collaborator

maxceem commented Sep 30, 2019

Website try to get the user-info about '305384'.
However, just a blank content return.
So the code request to the server again and again, due to the missUserId '305384'.

How we should do about this ?

We should request users only once. If, like in this situation we didn't get info about some user, we can show Unknown as you do now. Note, that you can check how we do on Project Listing page. After loading the list of projects, we request all the users once. If we cannot get some, we don't show them or show unknown.
So your approach is good, just have to request users one time only.

@jacky-zhangjb
Copy link
Contributor Author

@maxceem ,
I just update the code.
Please check it.
Thanks.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thanks @jankyzhang. All works great.

@maxceem maxceem merged commit ca65dce into topcoder-archive:cf19 Sep 30, 2019
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.

2 participants