[NEW] Improve room types API and usages #8274
Conversation
| @@ -164,7 +165,10 @@ Template.channelSettings.onCreated(function() { | |||
| save(value, room) { | |||
| let nameValidation; | |||
| if (!RocketChat.authz.hasAllPermission('edit-room', room._id) || (room.t !== 'c' && room.t !== 'p')) { | |||
There was a problem hiding this comment.
What if we treat all of the default room types as the other "custom" types and provide sane defaults for them? That way code like this can be merged into one line and not be nested?
There was a problem hiding this comment.
@graywolf336 I thought of that as well. I wanted to keep the footprint of this PR as small as possible, 100% compatible - and make the review easier.
There was a problem hiding this comment.
@mrsimpson in my opinion, I would rather take the time to review the pull request now than spend time in the future following the path of the code each time it has to be interacted with...if that makes sense.
There was a problem hiding this comment.
@graywolf336 This absolutely makes sense! I would propose to modify all places as I started it. Then, in one "final" commit, I would implement the defined methods for the inbuilt room-types. wdyt?
There was a problem hiding this comment.
@mrsimpson Whichever way is easier for you will work for me. 👍
There was a problem hiding this comment.
@graywolf336 I adapted the hard-coded values with interface calls.
|
I am liking where this is headed, will allow for easy customization for other people when they decide to fork Rocket.Chat and customize it. 👍 |
|
|
||
| if (!config.canBeDeleted) { | ||
| config.canBeDeleted = function(room) { | ||
| return Meteor.isServer ? |
There was a problem hiding this comment.
@graywolf336 Unfortunately, the client- and server-side-method-signatures vary.
I preferred having this switch in the generic code compared to having dedicated client-side-files or redundantly implementing this. WDYT?
|
@graywolf336 Can you please verify the merge-ability? I feel I'm done with the scope I need. |
|
@graywolf336 I implemented a tabbed creation dialog in assistify@3459fa8 . As discussed, I'll make that UI-stuff part of another PR once you accept this one ;)
|
mrsimpson
changed the title
…l over their definitions beyound the expected.
geekgonecrazy
changed the title
This allows for each different type of room to define which setting it wants to display instead of them being hard coded on the settings tab controller.
| @@ -1,4 +1,5 @@ | |||
| /* globals RocketChatTabBar */ | |||
| import { RocketChatTabBar } from 'meteor/rocketchat:lib'; | |||
There was a problem hiding this comment.
@graywolf336 well, it seems you're on a globals-killing-spree already ;)
There was a problem hiding this comment.
Heh, figured I would start the process ;)
| What is this file? Great question! To make Rocket.Chat more "modular" | ||
| and to make the "rocketchat:lib" package more of a core package | ||
| with the libraries, this index file contains the exported members | ||
| for the *client* pieces of code which does include the shared |
There was a problem hiding this comment.
@graywolf336 does that not affect the server as well?
There was a problem hiding this comment.
It does, but that file is specific to the client. The server has another one dedicated to it.
…ng custom room types
…r the user is authorized to create the room type
mrsimpson
force-pushed
the
core/#8264-room-types
branch
from
0481a5c to
b2b09a3
Compare
Provide implementation for the confirmation dialogs on leave and hide
(cherry picked from commit b463b84)
| if (Meteor.isClient) { | ||
| routeConfig.triggersExit = [roomExit]; | ||
| } | ||
| return FlowRouter.route(config.route.path, routeConfig); | ||
|
|
||
| return FlowRouter.route(roomConfig.route.path, routeConfig); |
There was a problem hiding this comment.
Just realized, this method say we return void yet here we return whatever FlowRouter.route returns. 🤔
What are your thoughts @mrsimpson on changing this method?
There was a problem hiding this comment.
I believe it should rerun void. It’s an anti pattern in some parts of the RC code to just make the last statement of a function return - without meaningful semantics
There was a problem hiding this comment.
E. G. Create a bunch of settings and the last statement is return this.add()
|
Closing in favor of #9009 so that we can merge it due to conflicts. |
Addresses #8246.
Custom room type creation UI is explicitly excluded from this PR as this will raise additional questions and is actually independent.
It's being implemented in #8264-room-types-creation-ui, PR will be started once this PR is merged to develop.
Motivation
I implemented custom room types for an enhancement in my
master. When testing, I found that I needed some modifications in generic functions (as it was coded to particular room types, probably legacy code).This PR shall make it possible that no modifications are necessary but that the
roomTyes-API generically being consumed.