[WIP] changeview/manage members dialog to use multiselect component consist… #100
Conversation
…ent with 'add member to team' dialog
|
Thanks @xjlim 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. |
| const user = Object.assign({}, users[i]); | ||
| user.value = user.id; | ||
| user.label = '@' + user.username; | ||
| users[i] = user; |
There was a problem hiding this comment.
A bit of an early review, but there's a few things to note here:
- We should never be mutating data that comes from the store. It should be treated as immutable even though JavaScript makes that hard to enforce it
- On top of that, nothing in the state should be changed without setState since React uses that to know if should redraw parts of the app
- The render function should never have any side effects (in this case, changing the state). It ideally should just take the state/props of the component and return whatever it renders as without touching or looking at anything else.
What you'll want to do here instead is, as you're filling up usersToDisplay, you'll also want to create a corresponding values array which has the corresponding IDs for each user in it, and then pass that to the MultiSelect
|
Right now for both view/manage dialog, user can search to filter the list and navigate up/down on the list. What is the expected behavior when user presses the enter key on the multiselect component? |
|
@xjlim ENTER should open the dropdown menu for the user that's in focus in the Manage Members modal, as it does now.
In View Members modal, ENTER shouldn't take any action. |
|
the up/down navigation might conflict with the dropdown up/down. should up/down not work for the dropdown then
|
|
@xjlim Yeah I think the up/down arrows should work on the dropdown. But you raise a good point as it's not obvious how you can get back to scrolling through the users list when focus is on the dropdown. Hmm. Do you have any thoughts on that? I'll think about it too, I hadn't thought about that case earlier. |
|
i was thinking that the user can 'esc'ape out of the dropdown, but 'esc' by default will dismiss the dialog. |
|
Maybe it doesn't make sense to use the multiselect for that modal? I know we have that on the mobile app, but you can only remove members with it so it's fine there. This has more options in a dropdown, so I don't know if it works here |
|
@hmhealey I don't think we'll use multiselect in the modal the same way. Functionally the only change should be that you can navigate through the users list with up/down arrow keys to change the selected user in the list. My understanding was that this was simplest to achieve by changing the "View/Manage Members" dialog to use the same UI component as "Add Members to Team". Is there a better way to achieve this? @xjlim Thanks for your patience here, |
|
@xjlim Terribly sorry for the late response here. Would it be feasible to have the escape key close the dropdown, without dismissing the dialog? I think that would be most natural. The second escape key would then dismiss the dialog. If that is not tehcnically feasible, we could consider other options like the left arrow key going back to the user list. Again, really apologize for the delay here @xjlim. I'll make sure to be more responsive. |
|
@xjlim Let me know if you have any questions about the above comment? |
|
@jasonblais I believe it won't be easy to do the first case as that involves changing bootstrap modal default escape behaviour. I will try the second option. |
|
Hey @xjlim , let me know if there's anything we can help with? |
|
@jasonblais Sorry, I have gotten busy. I will try to pick it up this weekend. I believed I was stuck trying to figure out how to toggle the dropdown of each row within the dialog and found that there are some inconsistency in the design. Some dialogs use the react bootstrap modal wrapper with props to show/hide while others use |
|
@xjlim No worries - if you have any further questions, don't hesitate to ask in the Contributors or Developers channel at https://pre-release.mattermost.com |
|
@xjlim Closing due to inactivity. If you'd still be interested to work on the ticket, let us know and we'd be more than happy to help. |
Include cleaning action_types folder
Include cleaning action_types folder
Summary
Change View/Manage members dialog to be consistent with 'Add members to team' dialog
Ticket Link
Issue
Jira
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]