Replace modal store with redux view reducer #6784 #371
Conversation
|
Thanks @mikelinden1 for the pull request! Per the CONTRIBUTING.md file displayed when you created this pull request, we need to add you to the list of approved contributors for the Mattermost project. Please help complete the Mattermost contribution license agreement? This is a standard procedure for many open source projects. Your form should be processed within 24 hours and reviewers for your pull request will be able to proceed. Please let us know if you have any questions. We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team. |
|
@mikelinden1 you've got a few styling errors (which is causing the build to fail). You can see them locally by running |
|
@jwilander Sorry about that! I ran check-style on the server repo by mistake. I just pushed the fixes. |
There was a problem hiding this comment.
Looks awesome aside from some nitpicking about import paths
The only thing I could see maybe changing is that passing in the modalId everywhere may be a bit redundant if every ToggleModalButtonRedux will take the exact same modalId for a given dialogType. If EditChannelHeaderModal, for example, is always passed in with ModalIdentifiers.EDIT_CHANNEL_HEADER, we could consider adding a static method/constant to each modal component that contains a string identifier that could be used by ToggleModalButtonRedux. Then, ToggleModalButtonRedux could do
const modalData = {
modalId: dialogType.MODAL_ID, // where EditChannelHeaderModal.MODAL_ID = ModalIdentifiers.EDIT_CHANNEL_HEADER
dialogProps,
dialogType
};
this.props.actions.openModal(modalData);That being said, that's just an idea since it may end up being better allowing modalId to be passed in separately since that does give us some options we wouldn't otherwise have.
Edit: Actually, that probably won't work due to connect or injectIntl potentially wrapping the modal component classes. Never mind then.
components/modal_controller/index.js
Outdated
| import {connect} from 'react-redux'; | ||
| import {bindActionCreators} from 'redux'; | ||
|
|
||
| import {closeModal} from '../../actions/views/modals'; |
There was a problem hiding this comment.
Can you change this to use an absolute path? We prefer to use those except in cases like below where you're importing the component into index.js
There was a problem hiding this comment.
No problem, I just pushed the change.
|
|
||
| import ModalToggleButtonRedux from './toggle_modal_button_redux.jsx'; | ||
|
|
||
| import {openModal, closeModal} from '../../actions/views/modals'; |
|
@lindalumitchell would you be able to give this a test pass? It affects the opening/closing of the edit channel header, view channel info, channel notifications, channel invite and delete channel modals |
|
@mikelinden1 @jwilander I've been testing a bit, and found one Javascript error:
Observed: Add Members modal does not open, Javascript error displays Note: There are some important elements I can't verify at the moment because they're broken on master:
Edit: Forgot screenshots: |
|
Found the problem. Just had to pass the openDialog function as the showInviteModal prop to ChannelMembersModal. @lindalumitchell @jwilander |
|
@mikelinden1 @jwilander Should I do another test pass at this point? |
|
Yes please @lindalumitchell, if you can just confirm the one bug fix and we should be good. Thanks! |
|
|
|
Spinmint test server created at: https://i-08ef1cc0053679432.spinmint.com Test Account 1: Email: Test Account 2: Email: Instance ID: i-08ef1cc0053679432 |
|
@mikelinden1 @jwilander I verified the issue has been fixed, and did a little bit of re-testing, and it looks good! One caveat: I'm still unable to test Edit Header, Edit Purpose, and Rename Channel from the drop-down, because those are still broken on master. So it looks like we'll need to verify those after this PR is merged, once those issues are fixed. |
|
Thanks @lindalumitchell ! @saturninoabril if you're happy with the changes from your feedback, please approve and we can merge |
|
Spinmint test server destroyed |
Summary
Added Redux reducer to toggle modal state. Use ToggleModalButtonRedux instead of ToggleModalButton and provide a modalId prop (defined in constants->modalIdentifiers). There is a ModalController component in the NeedsTeam component that displays the modals.
Ticket Link
mattermost/mattermost#6784
Checklist
make check-styleto check for style errors (required for all pull requests)make testto ensure unit and component tests passed