-
Notifications
You must be signed in to change notification settings - Fork 397
content: Change UserMention to render user mentions dynamically
#2087
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
base: main
Are you sure you want to change the base?
Conversation
UserMention to render user mentions dynamically
|
Thanks! This looks quite promising. Here's some high-level comments. Once you address these, the PR will be ready for a more detailed maintainer review. First, to your questions:
Yeah, including a fix for that is fine as long as it's a separate commit, which you've done.
Yeah, we don't want to add that sort of logic. It makes the code more complicated to read and think through, and it makes the behavior in tests diverge from the behavior in the real app. Instead, any existing tests that start needing a store widget should add one. Lots of our other existing tests provide a store widget, because lots of our other widget code requires one, so it should be possible to do this with very little added code. This change can happen as its own separate prep commit, before the change that makes it necessary. One of your later commits changes some existing tests so that they pass. Those changes should instead happen in the same commits that make them necessary. In general, if you check out any individual commit and run Another later commit adds tests for an earlier commit. Instead, those tests should be added in the same commit as the code they test. |
5b6a255 to
04c3012
Compare
Stores the user ID from the `data-user-id` HTML attribute. `userId` will be null in case of wildcard or legacy mentions. Changed 'UserMentionNode' calls in content model tests. Preparatory commit for user mention widget change.
`isSilent` indicates whether the mention is silent (without @ prefix), extracted from existing CSS parsing logic. This defaults to false for regular mentions. `isSilent` is necessary for zulip#647. Changed 'UserMentionNode' calls in content model tests. Preparatory commit for user mention widget change.
Added 'wrapWithPerAccountStoreWidget' parameter to 'testContentSmoke' and 'testFontWeight' tests. Preparatory commit for user mention widget and tests changes.
Look up current user name from PerAccountStore when rendering mentions, while respecting "isSilent" flag. This ensures mentions display correctly even if the user has changed their name since the message was sent. Falls back to original HTML text when user not found. Changed old user mention tests to pass store widget. Added 'dynamic name resolution' tests group. Fixes zulip#1258
|
Changes made:
Please review changes. Small question: Is it ok to tag maintainers, like so: @gnprice, when feedback is addressed to speed up the review process? The contributing guide doesn't specifically advise for or against it in the "How to help move the review process forward" section. |
|
Cool, thanks for the revision. This looks ready for a more detailed review.
It's not really harmful to tag maintainers on a PR thread, but doesn't have much effect. I already watch this repo and so get notifications for every update to every PR. More generally on GitHub, anyone who's already participating in the thread can be expected to be subscribed and so getting notifications for every update, with or without an @-mention. That means that GitHub @-mentions are mainly useful for pulling someone into a thread for the first time. The main thing to do when you've addressed feedback is just to post a comment clearly saying you've done so. That way when a maintainer looks at it, it's clear that it's ready for the next review and not still waiting for a revision from you. |
Fixes #1258
Changes:
Model:
Added userId, isSilent and updated parser.
Widget:
Modified widget to fetch username from store and handle isSilent flag.
Tests:
Updated parser tests and added new widget tests.
Doubts:
UPDATE: Removing the error causing annotations fixes the CI testing issue. However, since this code change is not related to issue #1258, please advise on how I should proceed.