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.

Sign up for GitHub

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

Jump to bottom

[PLT-6548] Mobile Web View: Add "Leave Team" option to main menu #6472

Merged
merged 2 commits into from Jun 19, 2017
Merged

[PLT-6548] Mobile Web View: Add "Leave Team" option to main menu #6472

merged 2 commits into from Jun 19, 2017

Conversation

cpanato
Copy link
Contributor

@cpanato cpanato commented May 22, 2017

Summary

Mobile Web View: Add "Leave Team" option to main menu

Ticket Link

JIRA: https://mattermost.atlassian.net/browse/PLT-6548
GitHub: #6460

Checklist

  • Has UI changes

@cpanato cpanato mentioned this pull request May 22, 2017
Closed
@cpanato
Copy link
Contributor Author

cpanato commented May 22, 2017

this is what I call teamworking :)

@jasonblais jasonblais added 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels May 22, 2017
@cpanato
Copy link
Contributor Author

cpanato commented May 22, 2017

PS. if you try to click on the leave team to leave the team you will get an javascript error. The error should be fixed in this PR: mattermost/mattermost-redux#107

@esethna esethna added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels May 23, 2017
esethna
esethna requested changes May 23, 2017
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.

Thanks @cpanato. Unfortunately I still seem to be getting a JS error even through the PR you mentioned has been merged and I started a new test server. Do you mind taking a look when you get a chance?

image

@esethna esethna added the Awaiting Submitter Action Blocked on the author label May 23, 2017
@cpanato
Copy link
Contributor Author

cpanato commented May 24, 2017

I dont know what more I need to do in order to get the fix from the other repo. Do we need to somehow update the dependencies for mattermost-redux ?

@esethna @jwilander

@cpanato
Copy link
Contributor Author

cpanato commented May 24, 2017

I rebased maybe it works

@jwilander
Copy link
Member

You need to point it at the latest redux changes. Just make this change or rebase after this PR goes in:
https://github.com/mattermost/platform/pull/6484/files#diff-fa5ac5b65b6fe8ff33bb9689b102faf7R4883

@cpanato
Copy link
Contributor Author

cpanato commented May 24, 2017

I will wait the PR @jwilander

@cpanato
Copy link
Contributor Author

cpanato commented May 24, 2017

actually looks like it is been merged :)

@jwilander jwilander added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels May 24, 2017
@esethna esethna added 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 May 24, 2017
@esethna
Copy link
Contributor

esethna commented May 24, 2017

Thanks @cpanato, looks like it still needs a rebase when you have a chance?

@esethna esethna added the Awaiting Submitter Action Blocked on the author label May 24, 2017
@jasonblais jasonblais removed the Awaiting Submitter Action Blocked on the author label May 26, 2017
@cpanato
Copy link
Contributor Author

cpanato commented May 29, 2017

@esethna @jasonblais rebased

@jasonblais jasonblais removed the Setup Old Test Server Triggers the creation of a test server label May 29, 2017
@esethna esethna added the Setup Old Test Server Triggers the creation of a test server label May 29, 2017
esethna
esethna approved these changes May 30, 2017
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.

Thanks @cpanato, looks good!

@esethna esethna added the 2: Dev Review Requires review by a developer label May 30, 2017
@esethna esethna removed 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels May 30, 2017
@esethna esethna added this to the v3.10.0 milestone May 30, 2017
@esethna esethna removed their assignment May 30, 2017
@jasonblais jasonblais modified the milestones: v3.11.0, v3.10.0 Jun 3, 2017
saturninoabril
saturninoabril requested changes Jun 5, 2017
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.

thanks @cpanato, just a minor comments. please see code review.

webapp/components/sidebar_right_menu.jsx Outdated
@@ -173,6 +173,7 @@ export default class SidebarRightMenu extends React.Component {
let joinAnotherTeamLink;
let isAdmin = false;
let isSystemAdmin = false;
const leaveTeamIcon = Constants.LEAVE_TEAM_SVG;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of assigning to a variable, it might be better to use the constant directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

webapp/components/sidebar_right_menu.jsx Outdated
>
<span
className='icon fa'
dangerouslySetInnerHTML={{__html: leaveTeamIcon}}
Copy link
Member

Choose a reason for hiding this comment

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

className fa may not be necessary.

please update to:

            className='icon'
            dangerouslySetInnerHTML={{__html: Constants.LEAVE_TEAM_SVG}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@saturninoabril
Copy link
Member

Thanks @cpanato!

@cpanato
Copy link
Contributor Author

cpanato commented Jun 14, 2017

@saturninoabril rebased

@jasonblais jasonblais added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a developer labels Jun 15, 2017
@coreyhulen coreyhulen merged commit fe48987 into mattermost:master Jun 19, 2017
@cpanato cpanato deleted the PLT-6548 branch June 20, 2017 06:38
@jasonblais jasonblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Jun 23, 2017
@lindalumitchell lindalumitchell added the Tests/Done Required release tests have been written label Jun 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coreyhulen coreyhulen coreyhulen 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/Done Required changelog entry has been written Docs/Not Needed Does not require documentation Tests/Done Required release tests have been written
Projects
None yet
Milestone
v4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@cpanato @jwilander @esethna @saturninoabril @coreyhulen @jasonblais @lindalumitchell @asaadmahmood