-
Notifications
You must be signed in to change notification settings - Fork 347
mentions : make @-mentions tappable to navigate to user profiles #1899
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
|
Thanks. Before we can review this PR, it will need a test, as described in the general instructions in the repo's README. |
ff98235 to
0722254
Compare
0722254 to
0194098
Compare
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.
Thanks @loveucifer! Sorry for delayed intial review. Comments below.
Also please update the tests in test/model/content_test.dart accordingly.
| final userId = node.userId; | ||
|
|
||
| final innerContent = InlineContent( | ||
| // If an @-mention is inside a link, let the @-mention override it. | ||
| recognizer: null, | ||
| // One hopes an @-mention can't contain an embedded link. | ||
| // (The parser on creating a UserMentionNode has a TODO to check that.) | ||
| linkRecognizers: null, | ||
|
|
||
| style: ambientTextStyle, | ||
|
|
||
| nodes: node.nodes); | ||
|
|
||
| if (userId != null && userId > 0) { | ||
| // Wrap with gesture detector if we have a valid user ID | ||
| return GestureDetector( | ||
| onTap: () => Navigator.push( | ||
| context, | ||
| ProfilePage.buildRoute(context: context, userId: userId), | ||
| ), | ||
| child: Container( | ||
| decoration: BoxDecoration( | ||
| // TODO(#646) different for wildcard mentions | ||
| color: contentTheme.colorDirectMentionBackground, | ||
| borderRadius: const BorderRadius.all(Radius.circular(3))), | ||
| padding: const EdgeInsets.symmetric(horizontal: 0.2 * kBaseFontSize), | ||
| child: innerContent, | ||
| ), | ||
| ); | ||
| } else { | ||
| // Regular container without gesture detector if no valid user ID | ||
| return Container( | ||
| decoration: BoxDecoration( | ||
| // TODO(#646) different for wildcard mentions | ||
| color: contentTheme.colorDirectMentionBackground, | ||
| borderRadius: const BorderRadius.all(Radius.circular(3))), | ||
| padding: const EdgeInsets.symmetric(horizontal: 0.2 * kBaseFontSize), | ||
| child: innerContent); | ||
| } |
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.
nit, this is doing the same right?
| final userId = node.userId; | |
| final innerContent = InlineContent( | |
| // If an @-mention is inside a link, let the @-mention override it. | |
| recognizer: null, | |
| // One hopes an @-mention can't contain an embedded link. | |
| // (The parser on creating a UserMentionNode has a TODO to check that.) | |
| linkRecognizers: null, | |
| style: ambientTextStyle, | |
| nodes: node.nodes); | |
| if (userId != null && userId > 0) { | |
| // Wrap with gesture detector if we have a valid user ID | |
| return GestureDetector( | |
| onTap: () => Navigator.push( | |
| context, | |
| ProfilePage.buildRoute(context: context, userId: userId), | |
| ), | |
| child: Container( | |
| decoration: BoxDecoration( | |
| // TODO(#646) different for wildcard mentions | |
| color: contentTheme.colorDirectMentionBackground, | |
| borderRadius: const BorderRadius.all(Radius.circular(3))), | |
| padding: const EdgeInsets.symmetric(horizontal: 0.2 * kBaseFontSize), | |
| child: innerContent, | |
| ), | |
| ); | |
| } else { | |
| // Regular container without gesture detector if no valid user ID | |
| return Container( | |
| decoration: BoxDecoration( | |
| // TODO(#646) different for wildcard mentions | |
| color: contentTheme.colorDirectMentionBackground, | |
| borderRadius: const BorderRadius.all(Radius.circular(3))), | |
| padding: const EdgeInsets.symmetric(horizontal: 0.2 * kBaseFontSize), | |
| child: innerContent); | |
| } | |
| Widget widget = Container( | |
| decoration: BoxDecoration( | |
| // TODO(#646) different for wildcard mentions | |
| color: contentTheme.colorDirectMentionBackground, | |
| borderRadius: const BorderRadius.all(Radius.circular(3))), | |
| padding: const EdgeInsets.symmetric(horizontal: 0.2 * kBaseFontSize), | |
| child: InlineContent( | |
| // If an @-mention is inside a link, let the @-mention override it. | |
| recognizer: null, // TODO(#1867) make @-mentions tappable, for info on user | |
| // One hopes an @-mention can't contain an embedded link. | |
| // (The parser on creating a UserMentionNode has a TODO to check that.) | |
| linkRecognizers: null, | |
| // TODO(#647) when self-user is non-silently mentioned, make bold, and: | |
| // TODO(#646) when self-user is non-silently mentioned, | |
| // distinguish font color between direct and wildcard mentions | |
| style: ambientTextStyle, | |
| nodes: node.nodes)); | |
| if (node.userId == null) return widget; | |
| return GestureDetector( | |
| onTap: () { | |
| Navigator.push(context, | |
| ProfilePage.buildRoute(context: context, userId: node.userId!)); | |
| }, | |
| child: widget); |
- Make userId required in UserMentionNode for explicit parameters - Reorder constructor parameters - Refactor UserMention widget to avoid code duplication - Move tests to content_test.dart
mentions: Enable navigation to user profiles on tap on chat threads
Before, tapping on an @-mention in a message had no effect. This
was inconsistent with tapping a sender's name or avatar, which
navigates to their profile in the thread . This change makes @-mentions tappable to
provide a consistent user experience and match web functionality as mentioned in the issue
The HTML parser extracts the
data-user-id" attribute from the mention'selement and stores it in theUserMentionNode. TheUserMentionwidget uses this "id" to wrap the text in aGestureDetector`,navigating to the user's profile page on tap.
Working Demo
zulip.mp4
Fixes : #1867