Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support MSC2876: Reading events from rooms #34

Merged
merged 1 commit into from
May 12, 2021
Merged

Support MSC2876: Reading events from rooms #34

merged 1 commit into from
May 12, 2021

Conversation

turt2live
Copy link
Member

turt2live added a commit to matrix-org/matrix-react-sdk that referenced this pull request May 4, 2021
@turt2live
Support UI for MSC2876: Widgets reading events from rooms
e044902 
MSC: matrix-org/matrix-spec-proposals#2876
Fixes element-hq/element-web#15747
Requires matrix-org/matrix-widget-api#34
@turt2live turt2live mentioned this pull request May 4, 2021
Merged
@turt2live turt2live requested a review from a team May 4, 2021 03:51
jryans
jryans approved these changes May 5, 2021
View reviewed changes
Copy link
Contributor

@jryans jryans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, but the directional naming is a bit confusing.

src/models/WidgetEventCapability.ts
@@ -19,6 +19,7 @@ import { Capability } from "..";
export enum EventDirection {
Send = "send",
Receive = "receive",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, could this concept be named listen instead...? "Receive" and "read" are quite similar... I guess it needs an MSC to change the capabilities though... 😅

If it's too late to actually rename, then at least add some comments... I have already been unsure each time I saw either one while reading this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rename the enum name, but the value would ideally stay the same (or be changed in that MSC, which hasn't landed yet, but it would require managing a tech debt episode we don't have availability for)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, actually it's not safe to rename the enum at this stage, but we should probably do that before a proper release of this SDK... Have filed #35 to track

@turt2live
Copy link
Member Author

landing this side given approval

@turt2live turt2live merged commit 5b32803 into master May 12, 2021
@turt2live turt2live deleted the travis/msc2876 branch May 12, 2021 16:37
@turt2live turt2live mentioned this pull request Aug 26, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jryans jryans jryans approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@turt2live @jryans