Refactor Modals, ChannelHeader and Navbar component

[cometkim]

Summary

• Refactored ChannelHeader and ChannelHeaderMobile
  • Moved to Redux store
  • Moved to Pure or SFC
  • Extracted duplicates to components and share it
  • Renamed NavBar to ChannelHeaderMobile for consistency
  • Changed to uses ModalController and Redux action instead of manage modal internally
    • explainer:
      
      Many modals was rendered twice, each in NavBar and ChannelHeader
  • Re-organized cases where dropdown menus can appear
  • Rewirte dropdown so it would more declarative and deterministic in code level
  • Removed unused/uncovered codes
  • Removed jQuery
• Changed ModalController to pure
• Refactored Modals
  • InviteMembersModal
  • ChannelNotificationsModal
• Renamed touched filenames to .js

Ticket Links

InviteMemberModal

• [Help Wanted] Migrate invite_member_modal.jsx to be pure and use Redux mattermost#7683

Navbar

• MM-11283
• [Help Wanted] [MM-11283] Migrate components/navbar/navbar.jsx to Redux and PureComponent mattermost#9093

Checklist

• 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 UI changes

Future plan

• Share mapStateToProps for ChannelHeader and ChannelHeaderMobile (like ChannelHeaderDropdown)
• Refactor every modals to make sure controlled by Redux action
• Research React patterns to increase the cohesion of props of dropdown menu

[cometkim]

I would prefer the NavBar to be renamed to ChannelHeaderMobile to avoid confusing, thoughts?

[jasonblais]

@cometkim Really appreciate your help here.

Let me know if you have any questions or would like someone to review progress so far?

Excited for this :)

[cometkim]

@jasonblais Still working on. I will request a review in a couple days.

[jasonblais]

Thanks @cometkim! The matrix is great. We'll review.

A question: Can you help clarify the difference between 'O', 'X' and '-'?

[cometkim]

@jasonblais Sorry to late,

• O means: menu item appear if this state is true
• X means: the menu item doesn't appear if this state is true
• - means: it doesn't matter for the menu item

you can arrange it in a better format to clarify.

What I want to do is to unify the visibility verification of menu items based on the channel state. (user permissions or system settings are not included hear)

if (isDefault) {
  return null;
}

if (isReadonly) {
 return null;
}

I can leave it with test codes, or make it more declarative in a flavor of JSX(ex, like ChannelPermissionGate component).

To do this, I need to know clearly conditions for visibility of each menu item. it's too hard to figure it out through the code.

[jasonblais]

Thanks @cometkim! I've added a QA label given they'll be better equipped to confirm the above matrix is correct.

[DHaussermann]

@cometkim Can you answer a couple more questions about your matrix above.

1. I'm not sure why some of these cells have a - instead of an X or an 0. For example NotificationPreferences are no visible on DM channels but are visible on Public, Private, and GMs but this this is not explicit on you matrix

2. Can you tell me more about default state?

[jasonblais]

@cometkim One other question:

3: Do you know if these changes can affect the Mattermost advanced permissions feature? https://docs.mattermost.com/deployment/advanced-permissions.html

Some of the menu items are hidden depending on which permissions are enabled for the user.

[cometkim]

@cometkim Can you answer a couple more questions about your matrix above.

1. I'm not sure why some of these cells have a - instead of an X or an 0. For example NotificationPreferences are no visible on DM channels but are visible on Public, Private, and GMs but this this is not explicit on you matrix
2. Can you tell me more about default state?

@DHaussermann

1. because the matrix came from statement code in Navbar and ChannelHeader component. but you right, I was missed DM statement in the matrix. you can check it to O or X if you think it need validate state explicitly.

2. Default channel is Town Square

Thank you!

[cometkim]

@cometkim One other question:

3: Do you know if these changes can affect the Mattermost advanced permissions feature? https://docs.mattermost.com/deployment/advanced-permissions.html

Some of the menu items are hidden depending on which permissions are enabled for the user.

@jasonblais I use ChannelPermissionGate and TeamPermissionGate in each menu item. I think it's way more declarative and easy to manage.

[cometkim]

I just removed 1000 lines of code from Navbar and it works properly. very excited

[jwilander]

Took a look through and I really like the direction you're going with this, it's very much more React-like and a much more organized and clean structuring 👍

[cometkim]

Thank for review @jwilander, I go for ChannelHeader.

BTW, I am wondering if I can express channel validation more declaratively using HOC or render props. something like...

as is:

if (isDefault) {
  return null;
}

to be (in parent):

<ValidateChannel {...whatever => (
  <MenuItem/>
)} >

Any idea here?

[cometkim]

This has been delayed for days, but I just restarted.

[jwilander]

I don't have a strong opinion either way if you want to use HOC or just return null early in the lower components. We don't use a ton of HOC currently, and I'd lean just slightly towards using the render props to reduce the hierarchy a bit but 0/5 on that

[jasonblais]

Excellent! Huge thanks @DHaussermann and @cometkim 🎉

@jwilander ready to merge, apart from a dev confirmation on changes since the latest dev review

[jwilander]

I'll give it another quick pass and then merge

[jwilander]

Changes look good to me, great work @cometkim !

[cometkim]

Finally...! 😇

Thanks @hmhealey @saturninoabril @jwilander @jasonblais for following-up and review this PR.
Especially @DHaussermann 's QA has been really a great help.

Let me know if have any problem or need any explanation