-
Notifications
You must be signed in to change notification settings - Fork 51
[PROD RELEASE] - Bug fixes #1648
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
Conversation
fix(PM-1137): search broken in projects list
fix(PM-1140): show spinner when changing between projects
| } | ||
| { | ||
| membersExist && ( | ||
| !isLoadingProject && membersExist && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider checking the isLoadingProject condition earlier in the logic to potentially avoid unnecessary checks or rendering. This might improve readability and performance if isLoadingProject is a common state.
| ) | ||
| } | ||
|
|
||
| {isLoadingProject && <Loader />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a conditional rendering check to ensure that the Loader component is only displayed when necessary. This will prevent unnecessary rendering and improve performance.
| projects: PropTypes.arrayOf(PropTypes.object), | ||
| projectMembers: PropTypes.arrayOf(PropTypes.object), | ||
| invitedMembers: PropTypes.arrayOf(PropTypes.object), | ||
| isLoadingProject: PropTypes.bool.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of isLoadingProject as a required prop is clear, but ensure that this prop is being used correctly within the component to handle loading states. Double-check if there are any related changes needed in the component logic to accommodate this new prop.
src/containers/Users/index.js
Outdated
| isLoadingProject: false | ||
| } | ||
| this.loadProject = this.loadProject.bind(this) | ||
| this.updateProjectNember = this.updateProjectNember.bind(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in method name updateProjectNember. It should likely be updateProjectMember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hentrymartin can you please commit a fix for this in develop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kkartunov I've added a fix for this!
| } | ||
|
|
||
| loadProject (projectId) { | ||
| this.setState({ isLoadingProject: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling potential errors from fetchProjectById to ensure isLoadingProject state is properly reset in case of failure.
| email: m.email || invitedUsers[m.userId].handle | ||
| })) | ||
| })), | ||
| isLoadingProject: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of isLoadingProject: false seems to be outside the scope of the invitedMembers mapping. Ensure that this property is intended to be part of the state update and not mistakenly included within the invitedMembers transformation.
| }) | ||
|
|
||
| const { user, updateProjectNember } = this.props | ||
| const { user, updateProjectMember } = this.props |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo fix: The variable updateProjectNember was corrected to updateProjectMember. Ensure that all references to this function are updated throughout the codebase to prevent any potential errors.
src/components/Users/index.js
Outdated
| loadProject: PropTypes.func.isRequired, | ||
| updateProjectNember: PropTypes.func.isRequired, | ||
| updateProjectMember: PropTypes.func.isRequired, | ||
| removeProjectNember: PropTypes.func.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo correction needed: removeProjectNember should be removeProjectMember to match the corrected spelling of updateProjectMember.
src/containers/Users/index.js
Outdated
| this.loadProject = this.loadProject.bind(this) | ||
| this.updateProjectNember = this.updateProjectNember.bind(this) | ||
| this.updateProjectMember = this.updateProjectMember.bind(this) | ||
| this.removeProjectNember = this.removeProjectNember.bind(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method removeProjectNember seems to have a typo in its name. Consider renaming it to removeProjectMember to maintain consistency with the corrected method name updateProjectMember.
| } | ||
|
|
||
| updateProjectNember (newMemberInfo) { | ||
| updateProjectMember (newMemberInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name updateProjectMember has been corrected from updateProjectNember. Ensure that all references to this function throughout the codebase are updated to prevent any potential errors.
src/containers/Users/index.js
Outdated
| loadProject={this.loadProject} | ||
| updateProjectNember={this.updateProjectNember} | ||
| updateProjectMember={this.updateProjectMember} | ||
| removeProjectNember={this.removeProjectNember} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name removeProjectNember seems to have a typo similar to the one corrected for updateProjectMember. Consider renaming it to removeProjectMember for consistency and clarity.
| if (this.state.isRemoving) { return } | ||
|
|
||
| const { removeProjectNember, invitedMembers } = this.props | ||
| const { removeProjectMember, invitedMembers } = this.props |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo fix: The variable removeProjectNember was corrected to removeProjectMember. Ensure that this change is consistent throughout the codebase where this variable is used.
| } | ||
|
|
||
| removeProjectNember (projectMember) { | ||
| removeProjectMember (projectMember) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo fix: The method name was corrected from removeProjectNember to removeProjectMember. Ensure that all references to this method throughout the codebase are updated accordingly to prevent any potential errors.
https://topcoder.atlassian.net/browse/PM-1137
https://topcoder.atlassian.net/browse/PM-1140