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

Update spec for MSC3952: intentional mentions #1508

Merged
merged 23 commits into from
May 17, 2023
Merged

Update spec for MSC3952: intentional mentions #1508

merged 23 commits into from
May 17, 2023

Conversation

clokep
Copy link
Contributor

@clokep clokep commented May 2, 2023 •
edited by github-actions bot
Loading

@clokep
Copy link
Contributor Author

clokep commented May 3, 2023

I think this is going to need a bit more work, but it is ready for an initial round of review.

@clokep clokep marked this pull request as ready for review May 3, 2023 19:45
@clokep clokep requested a review from a team as a code owner May 3, 2023 19:45
@clokep clokep mentioned this pull request May 9, 2023
Merged
@clokep @MTRNord
Fix syntax highlighting.
da39529 
Co-authored-by: Marcel <MTRNord@users.noreply.github.com>
anoadragon453
anoadragon453 previously requested changes May 11, 2023
View reviewed changes
Copy link
Member

@anoadragon453 anoadragon453 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall I think this looks great! I just have a few changes requested below.

Edit: Sorry, ignore this. It was for a GitHub demo :)

content/client-server-api/modules/event_replacements.md Show resolved Hide resolved
@anoadragon453 anoadragon453 requested a review from a team May 12, 2023 15:23
@turt2live turt2live added the release-blocker Blocks the next release from happening label May 16, 2023
@turt2live turt2live requested review from richvdh and dbkr May 16, 2023 04:50
richvdh
richvdh requested changes May 16, 2023
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

thanks @clokep! A few thoughts and comments

content/client-server-api/modules/mentions.md Outdated Show resolved Hide resolved
content/client-server-api/modules/mentions.md Outdated Show resolved Hide resolved
content/client-server-api/modules/mentions.md Outdated Show resolved Hide resolved
content/client-server-api/modules/push.md Outdated Show resolved Hide resolved
content/client-server-api/modules/mentions.md Outdated Show resolved Hide resolved
content/client-server-api/modules/mentions.md Outdated Show resolved Hide resolved
content/client-server-api/modules/push.md Show resolved Hide resolved
changelogs/client_server/newsfragments/1508.feature Show resolved Hide resolved
content/client-server-api/modules/event_replacements.md Outdated Show resolved Hide resolved
content/client-server-api/modules/mentions.md Outdated Show resolved Hide resolved
clokep and others added 4 commits May 16, 2023 10:49
@clokep @richvdh
Fix typo.
80e0700 
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@clokep @richvdh
Added-in tags.
993839a 
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@clokep @richvdh
Clarify room mentions.
ad5e0d8 
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@clokep
More added-in/changed-in tags.
e258034
@clokep
Copy link
Contributor Author

clokep commented May 16, 2023

@richvdh Thank you very much for the review! I'm quite sure this will need a bit more back and forth, but I've handled your comments or replied to them (I've left open the threads I consider unresolved). Please take another look at your leisure!

@clokep clokep requested a review from richvdh May 16, 2023 16:46
richvdh
richvdh reviewed May 17, 2023
View reviewed changes
content/client-server-api/modules/mentions.md Outdated Show resolved Hide resolved
content/client-server-api/modules/mentions.md Outdated Show resolved Hide resolved
content/client-server-api/modules/mentions.md Outdated Show resolved Hide resolved
content/client-server-api/modules/mentions.md Outdated Show resolved Hide resolved
clokep and others added 4 commits May 17, 2023 07:41
@clokep @richvdh
Fix syntax.
aa50679 
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@clokep
Switch syntax.
2144f68
@clokep
Bold & link.
1d2c5db
@clokep
Add note about encryption.
3e6fd39
@clokep clokep requested a review from richvdh May 17, 2023 11:52
@clokep
Copy link
Contributor Author

clokep commented May 17, 2023

@richvdh I've handled all those comments! I'm not sure if there's anything to do about the special behavior discussed in #1508 (comment), however.

@richvdh
Copy link
Member

richvdh commented May 17, 2023

I'm not sure if there's anything to do about the special behavior discussed in #1508 (comment), however.

Yes, this is probably the best we can do for now.

richvdh
richvdh approved these changes May 17, 2023
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

@richvdh richvdh merged commit 466acdf into matrix-org:main May 17, 2023
10 checks passed
@clokep clokep deleted the intentional-mentions branch May 17, 2023 12:17
This was referenced May 24, 2023
Merged
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anoadragon453 anoadragon453 anoadragon453 left review comments

@MTRNord MTRNord MTRNord left review comments

@richvdh richvdh richvdh approved these changes

@dbkr dbkr Awaiting requested review from dbkr

Assignees
No one assigned
Labels
release-blocker Blocks the next release from happening
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@clokep @richvdh @anoadragon453 @MTRNord @turt2live