-
Notifications
You must be signed in to change notification settings - Fork 109
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
Initial support for member suggestions #1631
Conversation
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.
You should probably ignore this file in the review, it's just taken from M3, with the bugs mentioned in the description fixed.
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.
Same here, you can also ignore this file for the review.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1631 +/- ##
===========================================
- Coverage 63.04% 59.31% -3.73%
===========================================
Files 1223 1249 +26
Lines 31465 32452 +987
Branches 6480 6659 +179
===========================================
- Hits 19837 19250 -587
- Misses 8639 10319 +1680
+ Partials 2989 2883 -106
☔ View full report in Codecov by Sentry. |
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
modifier = Modifier | ||
.fillMaxWidth(), | ||
) | ||
Column(modifier = Modifier.fillMaxWidth()) { |
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.
Maybe extract in a fun? it's starting to be big
@@ -354,4 +381,57 @@ class MessageComposerPresenter @Inject constructor( | |||
snackbarDispatcher.post(snackbarMessage) | |||
} | |||
} | |||
|
|||
@OptIn(FlowPreview::class) | |||
private fun CoroutineScope.processComposerSuggestions() = launch { |
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.
Maybe extract this logic in his own file too?
Also makes sure it's done on a background dispatcher?
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.
Small remarks otherwise LGTM
Since this probably won't make it as a whole feature for the next release, I'll add it behind a feature flag, and do the suggestions while I'm on it. |
- Extract composer & mention suggestions to their composable. - Extract mentions suggestions processing to its own class. - Add `MatrixRoom.canTriggerRoomNotification` function. - Update strings and conditions for displaying the `@room` mention. - Add more tests.
d94abef
to
4445172
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Type of change
Content
Added code to:
Also,
BottomSheetScaffold
has a couple of bugs that make it work poorly with nested scrolling components, so I had to fork it and fix it while they are fixed in the upstream component.Please note the design isn't final, so it might change in the future. That's also why some strings were hardcoded for now, until they're verified and we add them to localazy.
Motivation and context
Closes #1452.
Screenshots / GIFs
Included in the PR.
Tests
@
in the composer.Note that this only adds the searching for room members and related UI, but it doesn't actually add the selected mention to the RTE.
Tested devices
Checklist