mentions : make @-mentions tappable to navigate to user profiles #1899
Conversation
|
Thanks. Before we can review this PR, it will need a test, as described in the general instructions in the repo's README. |
loveucifer
force-pushed
the
feature/tappable-mentions
branch
from
ff98235 to
0722254
Compare
loveucifer
force-pushed
the
feature/tappable-mentions
branch
from
0722254 to
0194098
Compare
loveucifer
changed the title
rajveermalviya
left a comment
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks @loveucifer! Sorry for delayed intial review. Comments below.
Also please update the tests in test/model/content_test.dart accordingly.
lib/model/content.dart
Outdated
| const UserMentionNode({ | ||
| super.debugHtmlNode, | ||
| required super.nodes, | ||
| this.userId, |
There was a problem hiding this comment.
This should be required so that the parameters are explicit, especially in the tests.
| this.userId, | |
| required this.userId, |
There was a problem hiding this comment.
FWIW the reason I generally want the fields on content nodes (and other types from the API) to be required in the constructors is for the sake of the code other than the tests. It's because these generally don't have reasonable defaults outside the context of making test data — instead, the only reasonable value to to use is whatever comes from the actual data from the server, and there's usually not any call site where that would have a constant value like null.
In test data, by contrast, there's very commonly some reasonable "boring" value that would work for most tests without the reader of the test needing to care about what specific value it is. For those, defaults are good. The contrast where a lot of fields have good defaults for test data but no good defaults for the app's code is why example_data.dart has lots of functions that basically just wrap constructors for various API types but provide defaults.
| 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.
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
3 participants
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