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.

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

Refactor Modals, ChannelHeader and Navbar component #1666

Merged
merged 100 commits into from Nov 22, 2018

Conversation

@cometkim
Member

cometkim commented Sep 5, 2018

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:
        image
        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

Navbar

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

This comment has been minimized.

Member

cometkim commented Sep 7, 2018

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

@cometkim cometkim force-pushed the cometkim:mm-11283 branch 2 times, most recently from f83858b to 33dc9cd Sep 7, 2018

@cometkim

This comment was marked as outdated.

Member

cometkim commented Sep 11, 2018

Uhh,,. this is huge. 😭

Refactor Goals:

  • Move modal components in Navbar and ChannelHeader to ModalControllder instead of rendering twice.
  • Extract dropdown menu items to be component which has permission control locally.
  • Rename Navbar to MobileChannelHeader ChannelHeaderMobile or ChannelHeaderMobile for consistency.
  • Make the MobileChannelHeader and ChannelHeader to share same componentsfor dropdown menu items

So it would be like

in ChannelHeader:

{/* <ul> */}
<ChannelHeaderDropdown>
  {/* <Group> appends divider to list if have any children */}
  <ChannelHeaderDropdown.Group>
    {/* Each item is <li><button> with permission controll */}
    <ChannelHeaderDropdown.ViewInfo {...} />
    <ChannelHeaderDropdown.NotificationPreferences {...} />
  </ChannelHeaderDropdown.Group>
  <ChannelHeaderDropdown.Group>
    <ChannelHeaderDropdown.AddMembers {...} />
    <ChannelHeaderDropdown.ManageMembers {...} />
  </ChannelHeaderDropdown.Group>
  ...
</ChannelHeaderDropdown>

in ChannelHeaderMobile:

<ChannelHeaderDropdown>
    <ChannelHeaderDropdown.ViewInfo {...} />
    <ChannelHeaderDropdown.ViewPinnedPosts {...} />
    <ChannelHeaderDropdown.NotificationPreferences {...} />
    <ChannelHeaderDropdown.AddMembers {...} />
    <ChannelHeaderDropdown.ManageMembers {...} />
   ...
</ChannelHeaderDropdown>
@jasonblais

This comment has been minimized.

Member

jasonblais commented Sep 16, 2018

@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

This comment has been minimized.

Member

cometkim commented Sep 18, 2018

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

@cometkim

This comment was marked as outdated.

Member

cometkim commented Sep 18, 2018

@jasonblais Could you check if this matrix has entered with correct conditions?

When the user has a right privilege to see the menu item, and all system settings are enabled, fill O or X to indicate visibility by state.
or just - if the menu always visible regardless of the state.

menu / state default public private DM GM archived readonly mobile desktop
ViewChannelInfo - - - X X - - - -
ViewPinnedPosts - - - X X - - O X
NotificationPreferences - - - X X X - - -
ToggleFavorite - - - X X - - O X
SetChannelHeader - - - - - X - - -
SetChannelPurpose - - - X X X - - -
AddMembers X - - X - X - - -
ViewMembers - - - X X - - - -
ManageMembers X - - X X X - - -
ConvertChannel X O X X X X - - -
RenameChannel X - - X X X - - -
DeleteChannel X - - X X X - - -
LeaveChannel X - - X X X - - -
WebRTC X X X O X - - - -
@jasonblais

This comment has been minimized.

Member

jasonblais commented Sep 18, 2018

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

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

@cometkim cometkim force-pushed the cometkim:mm-11283 branch from 688d42e to 43a681a Sep 20, 2018

@cometkim

This comment has been minimized.

Member

cometkim commented Sep 20, 2018

@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

This comment has been minimized.

Member

jasonblais commented Sep 20, 2018

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

@cometkim cometkim force-pushed the cometkim:mm-11283 branch from 50e191f to 8d67e9a Sep 20, 2018

@DHaussermann

This comment has been minimized.

DHaussermann commented Sep 20, 2018

@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

This comment has been minimized.

Member

jasonblais commented Sep 21, 2018

@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 jasonblais referenced this pull request Sep 21, 2018

Merged

Upgrade to babel 7 #579

3 of 3 tasks complete
@cometkim

This comment has been minimized.

Member

cometkim commented Sep 21, 2018

@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

This comment has been minimized.

Member

cometkim commented Sep 21, 2018

@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.

@DHaussermann

This comment was marked as outdated.

DHaussermann commented Sep 24, 2018

Hi @cometkim

Yes. I feel like it would be worth going through and explicit;y setting each cell to "0" or "X" unless we're certain these would not be affected by the code change. I can try and come back to this later today to create a version where all the values are explicit.

Regarding the accuracy of the data in the table above, unless I'm misunderstanding something, It seems like some of the "X" values in the DM and GM columns should in fact be "O". Please see my changes in bold and confirm if these are now correct.

menu / state default public private DM GM archived readonly mobile desktop
ViewChannelInfo O O O X X - - - -
ViewPinnedPosts - - - O O - - O X
NotificationPreferences - - - X O X - - -
ToggleFavorite - - - O O - - O X
SetChannelHeader - - - - - X - - -
SetChannelPurpose - - - X X X - - -
AddMembers X - - X - X - - -
ViewMembers - - - X X - - - -
ManageMembers X - - X X X - - -
ConvertChannel X O X X X X - - -
RenameChannel X - - X X X - - -
DeleteChannel X - - X X X - - -
LeaveChannel X - - X X X - - -
WebRTC X X X O X - - - -

Much appreciated.

@cometkim cometkim force-pushed the cometkim:mm-11283 branch from 948c212 to ec4439b Sep 24, 2018

@cometkim

This comment has been minimized.

Member

cometkim commented Sep 25, 2018

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

@cometkim cometkim force-pushed the cometkim:mm-11283 branch 4 times, most recently from fcb66ea to 8ac4d73 Sep 25, 2018

@jwilander

This comment has been minimized.

Member

jwilander commented Sep 27, 2018

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

This comment has been minimized.

Member

cometkim commented Sep 27, 2018

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 comment has been minimized.

Member

cometkim commented Oct 2, 2018

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

@jwilander

This comment has been minimized.

Member

jwilander commented Oct 2, 2018

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

This comment has been minimized.

Member

jasonblais commented Nov 22, 2018

Excellent! Huge thanks @DHaussermann and @cometkim 🎉

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

@jwilander

This comment has been minimized.

Member

jwilander commented Nov 22, 2018

I'll give it another quick pass and then merge

@jwilander

Changes look good to me, great work @cometkim !

@jwilander jwilander merged commit a0b36ae into mattermost:master Nov 22, 2018

2 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
mattermost/serverless-ops/github PR is up-to-date.

@cometkim cometkim deleted the cometkim:mm-11283 branch Nov 22, 2018

@cometkim

This comment has been minimized.

Member

cometkim commented Nov 22, 2018

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

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

@mattermost mattermost deleted a comment from mattermod Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment