-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add support to customize UI components rendering #12
Conversation
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
This PR relates to this draft in matrix-react-sdk: matrix-org/matrix-react-sdk#10382 Could you please review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sane
Co-authored-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code looks fair, but I'm lacking a bit of context here
- It appears that those UI customisations are already possible today. Why advantages do you see with the approach you're suggesting?
- How can we ensure that files stay in sync across codebases?
/** | ||
* Components that lead to a user being invited. | ||
*/ | ||
InviteUsers = "UIComponent.sendInvites", | ||
|
||
/** | ||
* Components that lead to a room being created that aren't already | ||
* guarded by some other condition (ie: "only if you can edit this | ||
* space" is *not* guarded by this component, but "start DM" is). | ||
*/ | ||
CreateRooms = "UIComponent.roomCreation", | ||
|
||
/** | ||
* Components that lead to a Space being created that aren't already | ||
* guarded by some other condition (ie: "only if you can add subspaces" | ||
* is *not* guarded by this component, but "create new space" is). | ||
*/ | ||
CreateSpaces = "UIComponent.spaceCreation", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe a more appropriate avenue for this kind of feature would be to piggy back off user permissions on a server as described in matrix-org/synapse#7731
I appreciate this issue has not received a lot of attention in recent times, but it seems like a more forward looking way to handle this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably could be used if implemented already, but it seems just and idea at the moment. But we would like some users (guests with special user ids) not to be able to invite, create rooms or spaces. Would be good to use a module API for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already possible using a feature flag UIFeature.widgets
as defined in UIFeature.ts
. See settings.md
in the matrix-react-sdk
for usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UIFeature.widgets
seems to toggle rendering of the integration manager. How should this be connected with InviteUsers, CreateRooms, CreateSpaces
functionality to be hidden or shown based on user ids? Could you please clarify?
matrixUserId: string | ||
) => void; | ||
|
||
export enum UIComponent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seem to be a copy/paste of UIFeature.ts
in the matrix-react-sdk
.
Before considering this i'd want to explore ways to keep the two in sync. I'm actually unsure what value this brings over the existing possible customisations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is true, I have mentioned this in my first comment. Maybe a solution could be to change react-sdk to use a UIComponent
from this PR? Or any other idea?
Based on this matrix-org/matrix-react-sdk#9931 (comment) we may think that component visibility customization API is deprecated in favor of module API.
This solution would simplify our deployment, maybe quite a lot. We plan to put all visibility customizations and landing for guests in a separate module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for replying to the comments in line. I don't think you've adressed the two questions asked in the original review i gave to this PR.
I'd be interested to hear more about this, before that moves forward
@germain-gg please find my answers/comments:
Based on this matrix-org/matrix-react-sdk#9931 (comment) we may think that component visibility customization API is deprecated in favor of module API. In this PR This solution could simplify our deployment. We plan to put all visibility customizations logic and landing for guests into a separate module.
A solution could be to change |
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
@germain-gg I changed this PR a bit, could you please have a look? Maybe PR can be moved forward. |
@dhenneke / @maheichyk just checking, is this still something you're hoping to get landed? |
# Conflicts: # README.md # src/lifecycles/types.ts
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
@Johennes yes would be great to have it, thanks for you message! I have updated the branch to latest main changes. |
@maheichyk many apologies for the delay you had to endure here so far. We discussed this internally today with @t3chguy, @andybalaam and @clokep. The approach of implementing this via the module API as you did is not unreasonable. However, we think there is an alternative to this that leverages Synapse and, with limited complexity, would allow exposing this feature on all clients while offering a single point of configuration. The CS-API already includes the authenticated What would be needed to implement this is the following:
We can help drafting the MSC and review pull requests but would need you to direct your development efforts to the other three points. Can you please let us know if this option seems viable to you? |
@Johennes thanks for the details, we will discuss this internally and inform you within next few days. |
@Johennes we are using existing deprecated API to customize rendering of components and are not blocked by this PR. In general the idea is good, but we don't plan to work on this topic at the moment. |
Thanks for getting back @maheichyk. Makes sense. I'll close this for now then. |
This PR suggest to extend module API with possibility to control rendering of UI Components.