9091 migrated leave_team_modal component to redux #1514

pradeepmurugesan · 2018-08-03T08:42:56Z

Summary

migrated leave_team_modal component to redux

Ticket Link

mattermost/mattermost#9091

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
Added or updated unit tests (required for all new features)
Has server changes (please link)
Has redux changes (please link)
Has UI changes
Includes text changes and localization file (.../i18n/en.json) updates
Touches critical sections of the codebase (auth, posting, etc.)

saturninoabril

Should convert this as well to make use of ToggleModalButtonRedux
showLeaveTeamModal can be removed.
TOGGLE_LEAVE_TEAM_MODAL can be removed as well

Great work on unit testing!

saturninoabril · 2018-08-06T07:46:40Z

components/leave_team_modal/index.js

+import {emitLeaveTeam, toggleSideBarRightMenuAction} from 'actions/global_actions.jsx';
+import {ModalIdentifiers} from 'utils/constants';
+
+import {getIsBusy} from '../../selectors/webrtc';


Should change to selectors/webrtc

saturninoabril · 2018-08-06T07:59:08Z

components/leave_team_modal/leave_team_modal.jsx


- if (WebrtcStore.isBusy()) {
+ if (this.props.isBusy) {
 WebrtcStore.emitChanged({action: WebrtcActionTypes.IN_PROGRESS});


I suggest this to convert as well into redux action.

I have a question regarding this. Even if we convert this to redux action , I assume somewhere down the chain we need to emit

WebrtcStore.emitChanged({action: WebrtcActionTypes.IN_PROGRESS});

so that the webrtcStore can act on it.

Where will that happen ? Kindly clarify, if I am getting it wrong .

Like on our offline DM, webrtcstore conversion to redux store is good to have in a separate PR. I'm good with not changing WebrtcStore.emitChanged.

saturninoabril · 2018-08-06T08:00:05Z

components/leave_team_modal/leave_team_modal.jsx

+
+ constructor(props) {
+ super(props);
+ this.handleSubmit = this.handleSubmit.bind(this);
 }

 handleSubmit(e) {


Please change this into arrow function: handleSubmit = (e) => {...}

saturninoabril · 2018-08-06T08:00:21Z

components/leave_team_modal/leave_team_modal.jsx

+ }),
+ };
+
+ constructor(props) {


You may remove this constructor.

saturninoabril · 2018-08-06T08:16:03Z

components/leave_team_modal/leave_team_modal.jsx

- var currentUser = UserStore.getCurrentUser();
-
- if (currentUser != null) {
+ if (this.props.currentUser != null) {


I think we don't need this condition check.

saturninoabril · 2018-08-06T08:22:58Z

tests/redux/actions/views/webrtc.test.js

+import thunk from 'redux-thunk';
+
+import {updateBusyWebrtc, initWebrtc, closeWebrtc} from 'actions/views/webrtc';
+import {ActionTypes} from '../../../../utils/constants';


Should change to utils/constants

saturninoabril · 2018-08-06T08:35:19Z

components/leave_team_modal/index.js

+ const modalId = ModalIdentifiers.LEAVE_TEAM;
+ const currentUser = getCurrentUser(state);
+ const isBusy = getIsBusy(state);
+ const show = state.views.modals.modalState[modalId] && state.views.modals.modalState[modalId].open;


Can we add selector for open state of the modalId?

saturninoabril · 2018-08-06T08:41:38Z

components/leave_team_modal/index.js

+import {connect} from 'react-redux';
+import {getCurrentUser} from 'mattermost-redux/selectors/entities/users';
+
+import {emitLeaveTeam, toggleSideBarRightMenuAction} from 'actions/global_actions.jsx';


Instead of using emitLeaveTeam, please use directly the redux action import {removeUserFromTeam} from 'mattermost-redux/actions/teams';

Doing so, you may delete that global actions' emitLeaveTeam.

saturninoabril · 2018-08-06T08:51:18Z

components/leave_team_modal/index.js

+ const isBusy = getIsBusy(state);
+ const show = state.views.modals.modalState[modalId] && state.views.modals.modalState[modalId].open;
+ return {
+ currentUser,


Just need to pass currentUserId and teamId

saturninoabril · 2018-08-06T08:55:40Z

@jasonblais I tested this locally and I found that hitting enter after opening the LeaveTeamModal does nothing. Current behavior makes the Yes button event and fires up action in leaving a team. You might want to give a quick smoke test just in case. Thanks!

pradeepmurugesan · 2018-08-06T09:33:12Z

@saturninoabril thanks for the review.. Will work on the comments and let you know..

lindy65 · 2018-08-06T17:30:22Z

Thanks @pradeepmurugesan @jasonblais

Tested the PR on the spinmint:

as the sysadmin and as user-1
Working as expected:
- Clicking on Yes to confirm leaving the team
- Clicking on No to cancel out of leaving the team
- If Allow any user with an account on this server to join this team was set to yes, I am able to rejoin the team from the Join another team menu option as well as by clicking on the + in the team sidebar.
- If Allow any user with an account on this server to join this team was set to no, I do not see the team in the list of teams available to join from the Join another team menu option or when clicking on the + in the team sidebar.
Issue found:
- same issue as saturninoabril where hitting enter after opening the LeaveTeamModal does nothing

saturninoabril

Looking good, just some comments.

Also, LeaveTeamModal on mobile browser's view (on sidebar right menu) is not working, you might have missed my last comments on that:

Should convert this as well to make use of ToggleModalButtonRedux
showLeaveTeamModal can be removed.
TOGGLE_LEAVE_TEAM_MODAL can be removed as well

saturninoabril · 2018-08-07T06:41:22Z

components/leave_team_modal/leave_team_modal.jsx

+ <button
+ type='button'
+ className='btn btn-danger'
+ onClick={this.handleSubmit.bind(this)}


No need to bind and should be fine to change into: onClick={this.handleSubmit}

I had to bind since I am accessing this.props inside handleSubmit . Without binding I am getting props undefined..

Kindly let me know if I am missing something

That should work since you've already change it into arrow function (handleSubmit = (e) => {...}) but I could be wrong in this case. Could you please try again and validate? Thanks

saturninoabril · 2018-08-07T06:43:22Z

components/leave_team_modal/leave_team_modal.jsx

+ toggleSideBarRightMenu: PropTypes.func.isRequired,
+ }),
+ };
+


Should add event listener on keypress to make hitting enter key works on this modal, could be like:

import WebrtcStore from 'stores/webrtc_store.jsx'; -import {WebrtcActionTypes} from 'utils/constants.jsx'; +import Constants, {WebrtcActionTypes} from 'utils/constants.jsx'; +import {isKeyPressed} from 'utils/utils.jsx'; class LeaveTeamModal extends React.PureComponent { static propTypes = { @@ -58,6 +59,22 @@ class LeaveTeamModal extends React.PureComponent { }), }; + componentDidMount() { + if (this.props.show) { + document.addEventListener('keypress', this.handleKeyPress); + } + } + + componentWillUnmount() { + document.removeEventListener('keypress', this.handleKeyPress); + } + + handleKeyPress = (e) => { + if (isKeyPressed(e, Constants.KeyCodes.ENTER)) { + this.handleSubmit(e); + } + } + handleSubmit = (e) => { this.props.onHide();

saturninoabril

Looks good to me, just one last bit :)
Thanks @pradeepmurugesan!

saturninoabril · 2018-08-10T07:54:59Z

components/leave_team_modal/leave_team_modal.jsx

+import WebrtcStore from 'stores/webrtc_store.jsx';
+import {WebrtcActionTypes} from 'utils/constants.jsx';
+import {isKeyPressed} from 'utils/utils';
+import Constants from 'utils/constants';


utils/constants is already declared above, should just combine those.

jwilander

Awesome

* 9140 migrated leave_team_modal component to redux * 9091 added tests for existing code * 9091 fixed code review comments * 9091 fixed the mobile view and added a key press listener * 9091 merged the duplicate import

pradeepmurugesan changed the title ~~[WIP] - 9140 migrated leave_team_modal component to redux~~ 9140 migrated leave_team_modal component to redux Aug 3, 2018

jasonblais requested review from jwilander and saturninoabril August 3, 2018 10:45

jasonblais added the 2: Dev Review Requires review by a core commiter label Aug 3, 2018

pradeepmurugesan force-pushed the GH-9091 branch from 4e739da to 20693f1 Compare August 6, 2018 08:02

saturninoabril suggested changes Aug 6, 2018

View reviewed changes

pradeepmurugesan changed the title ~~9140 migrated leave_team_modal component to redux~~ 9091 migrated leave_team_modal component to redux Aug 6, 2018

jasonblais added 3: QA Review Requires review by a QA tester Setup Old Test Server Triggers the creation of a test server labels Aug 6, 2018

lindy65 removed the 3: QA Review Requires review by a QA tester label Aug 6, 2018

saturninoabril removed the Setup Old Test Server Triggers the creation of a test server label Aug 7, 2018

mattermost deleted a comment from mattermod Aug 7, 2018

saturninoabril suggested changes Aug 7, 2018

View reviewed changes

saturninoabril approved these changes Aug 10, 2018

View reviewed changes

pradeepmurugesan added 5 commits August 13, 2018 07:50

9140 migrated leave_team_modal component to redux

f338688

9091 added tests for existing code

cf285ee

9091 fixed code review comments

f1cc0f5

9091 fixed the mobile view and added a key press listener

f2fe63c

9091 merged the duplicate import

9009e95

pradeepmurugesan force-pushed the GH-9091 branch from 246e4c0 to 9009e95 Compare August 13, 2018 05:50

jwilander approved these changes Aug 13, 2018

View reviewed changes

jwilander added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Aug 13, 2018

jwilander merged commit 6c4eb13 into mattermost:master Aug 13, 2018

lindalumitchell added the Tests/Not Needed Does not require new release tests label Aug 14, 2018

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

9091 migrated leave_team_modal component to redux #1514

9091 migrated leave_team_modal component to redux #1514

pradeepmurugesan commented Aug 3, 2018

saturninoabril left a comment

saturninoabril Aug 6, 2018

saturninoabril Aug 6, 2018

pradeepmurugesan Aug 6, 2018

saturninoabril Aug 6, 2018

saturninoabril Aug 6, 2018

saturninoabril Aug 6, 2018

saturninoabril Aug 6, 2018

saturninoabril Aug 6, 2018

saturninoabril Aug 6, 2018

saturninoabril Aug 6, 2018

saturninoabril Aug 6, 2018

saturninoabril commented Aug 6, 2018

pradeepmurugesan commented Aug 6, 2018

lindy65 commented Aug 6, 2018

saturninoabril left a comment •

edited

Loading

saturninoabril Aug 7, 2018

pradeepmurugesan Aug 7, 2018

pradeepmurugesan Aug 7, 2018

saturninoabril Aug 8, 2018

saturninoabril Aug 7, 2018 •

edited

Loading

pradeepmurugesan Aug 7, 2018

saturninoabril left a comment

saturninoabril Aug 10, 2018

jwilander left a comment

9091 migrated leave_team_modal component to redux #1514

9091 migrated leave_team_modal component to redux #1514

Conversation

pradeepmurugesan commented Aug 3, 2018

Summary

Ticket Link

Checklist

saturninoabril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saturninoabril commented Aug 6, 2018

pradeepmurugesan commented Aug 6, 2018

lindy65 commented Aug 6, 2018

saturninoabril left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saturninoabril Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saturninoabril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwilander left a comment

Choose a reason for hiding this comment

saturninoabril left a comment •

edited

Loading

saturninoabril Aug 7, 2018 •

edited

Loading