Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

loader for user modals #630

Merged
merged 3 commits into from
Feb 6, 2018
Merged

loader for user modals #630

merged 3 commits into from
Feb 6, 2018

Conversation

sudheerDev
Copy link
Contributor

@sudheerDev sudheerDev commented Jan 19, 2018 •
edited
Loading

Summary

Adding loader for users modal

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed
  • Has UI changes

@sudheerDev sudheerDev changed the title WIP WIP: users modal loader Jan 19, 2018
@jasonblais jasonblais added the Work in Progress Not yet ready for review label Jan 19, 2018
@sudheerDev sudheerDev changed the title WIP: users modal loader Users modal loader Jan 22, 2018
@sudheerDev
Copy link
Contributor Author

@jasonblais Done with my changes

@jasonblais jasonblais added 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server and removed Work in Progress Not yet ready for review labels Jan 22, 2018
@esethna
Copy link
Contributor

esethna commented Jan 26, 2018 •
edited
Loading

Thanks @sudheerDev, can you help me understand the details of this change so we can test it?

@sudheerDev
Copy link
Contributor Author

@esethna This PR add's a loader for more direct messages modal

jan-28-2018 20-27-09

esethna
esethna approved these changes Jan 29, 2018
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @sudheerDev!

@esethna esethna added 2: Dev Review Requires review by a core commiter and removed 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Jan 29, 2018
saturninoabril
saturninoabril reviewed Jan 30, 2018
View reviewed changes
components/more_direct_channels/more_direct_channels.jsx Outdated
} else {
this.props.actions.getProfilesInTeam(TeamStore.getCurrentId(), 0, USERS_PER_PAGE * 2);
this.props.actions.getProfilesInTeam(TeamStore.getCurrentId(), 0, USERS_PER_PAGE * 2).then(() => {
this.toggleChannelLoadingState();
Copy link
Member

@saturninoabril saturninoabril Jan 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why not just passed a value (true or false) instead of passing undefined? Maybe like

this.setChannelLoadingState(!this.state.loadingChannels);

saturninoabril
saturninoabril suggested changes Jan 30, 2018
View reviewed changes
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

LGTM, just minor comments, thanks!

components/more_direct_channels/more_direct_channels.jsx Outdated
} else {
this.props.actions.getProfilesInTeam(page + 1, USERS_PER_PAGE);
this.props.actions.getProfilesInTeam(page + 1, USERS_PER_PAGE).then(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Does the getProfilesInTeam first argument should be teamId (ref)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, It should be.

components/more_direct_channels/more_direct_channels.jsx Outdated
@@ -175,9 +186,13 @@ export default class MoreDirectChannels extends React.Component {
handlePageChange(page, prevPage) {
if (page > prevPage) {
if (this.listType === 'any') {
Copy link
Member

Choose a reason for hiding this comment

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

This has similar signature as above (LOC 80-88). Might be better to be on it's own function.

@lindy65 lindy65 added the Awaiting Submitter Action Blocked on the author label Jan 31, 2018
jwilander
jwilander approved these changes Jan 31, 2018
View reviewed changes
Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

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

LGTM other than @saturninoabril's comments

@sudheerDev
Copy link
Contributor Author

@saturninoabril Been a little occupied. Will address the comments soon

@saturninoabril
Copy link
Member

@sudheerDev No problem, thanks!

@sudheerDev sudheerDev changed the title Users modal loader [WIP]: Users modal loader Feb 2, 2018
@sudheerDev
Copy link
Contributor Author

I added a couple of loaders channel invite modal and add users to team along with direct messages. This might have to go through PM review again.

@sudheerDev sudheerDev changed the title [WIP]: Users modal loader loader for user modals Feb 5, 2018
@mattermost mattermost deleted a comment from mattermod Feb 6, 2018
@saturninoabril saturninoabril added 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server and removed Awaiting Submitter Action Blocked on the author Setup Old Test Server Triggers the creation of a test server labels Feb 6, 2018
@mattermost mattermost deleted a comment from mattermod Feb 6, 2018
@mattermost mattermost deleted a comment from mattermod Feb 6, 2018
@mattermod
Copy link
Contributor

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

@saturninoabril saturninoabril added the Setup Old Test Server Triggers the creation of a test server label Feb 6, 2018
@mattermod
Copy link
Contributor

Spinmint test server created at: http:https://i-0ff4478082770a529.spinmint.com

Test Admin Account: Email: [email protected] | Password: user-0

Test User Account: Email: [email protected] | Password: user-1

Instance ID: i-0ff4478082770a529

saturninoabril
saturninoabril approved these changes Feb 6, 2018
View reviewed changes
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @sudheerDev!

@saturninoabril saturninoabril added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Feb 6, 2018
@mattermod
Copy link
Contributor

Spinmint test server destroyed

@saturninoabril saturninoabril merged commit 3be3403 into mattermost:master Feb 6, 2018
@sudheerDev sudheerDev deleted the loader/directMessage branch February 6, 2018 10:47
@sudheerDev
Copy link
Contributor Author

No problem, Let me know if you find issues in this. I can help out.

@saturninoabril
Copy link
Member

I did a quick test and it still looks good. We'll have it battle-tested at pre-release :). Thanks!

@lindalumitchell lindalumitchell added the Tests/Not Needed Does not require new release tests label Feb 6, 2018
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Feb 9, 2018
@sudheerDev sudheerDev mentioned this pull request Apr 6, 2018
Merged
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jwilander jwilander jwilander approved these changes

@saturninoabril saturninoabril saturninoabril approved these changes

@esethna esethna esethna approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Not Needed Does not require new release tests
Projects
None yet
Milestone
No milestone
9 participants
@sudheerDev @esethna @saturninoabril @mattermod @jwilander @lindy65 @jasonblais @lindalumitchell @amyblais