Chat functionality #60
Conversation
|
|
|
We would need extend TimelineStream class functionality so we can push messages in a conversation, ideally some function: https://docs.effektio.org/api/main/dart-sdk/effektio/TimelineStream-class.html |
|
According to #3, the tasks have been implemented and can be reviewed in latest commit of PR. In general the basic chat is integrated. @gnunicorn you can have a look at CI check, then we can merge the PR in main. Some things which haven't been implemented :
|
There was a problem hiding this comment.
Thanks heaps, this takes us a lot further @gtalha07 ! I have a few remarks, some general, some we should fix before the merge others, we want to convert into tickets and fix later.
General remarks: please stick to one atomic change at a time, drive-by-fixes increase the PR size make it a lot harder to comprehend what is going and why. Instead for things like the style-changes or the logo-update just quickly jump into main branch off, do the fix, raise the PR and merge that branch into yours. Git-merging allows for a quick and easy way to keep that fine and your work fast - and this makes reviewing the distinct parts a lot easier. For this, let's keep it for now (aside from the stuff mentioned below) - but in the future let's stick to that style.
Please fix the following, then we can merge:
- clean up the code to remove outcommented stuff - just doesn't belong into
mainever (I've not remarked every instance, please go through and fix it yourself) - please revert the new-hamburger-logo-change: it's the wrong icon (and reopen the ticket belonging to it)
- Clarify/remove the wrapping function calls
- add a snackbar info to the user that the images/file they've added to the chat isn't actually send to the server right now
TThe following, we keep the code as is (maybe add FIXME's where I asked about that) but please convert the specific remarks into separate issues (all tagged bug and s-chat):
- localisation
- colors and theming
- sending image messages
- sending file messages
- creating reusable avatar-component
- showing the correct user-icon within the chat
- fixing the backpaging in chat
- user_id -> User.display_name()
- replacing our own time calculation with
time-module - recursion -> loop for listening to new messages
- the send-receive-message-flow
| fontSize: 20, | ||
| fontWeight: FontWeight.w700, | ||
| color: Colors.white, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| String getNameFromId(String name) { |
There was a problem hiding this comment.
Note to self: The actual native UserId-type already has facilities for this, we should move away from string to returning the native type(s) instead.
Further more, all users are bound to profiles that should actually have a display_name() instead - that might be the username but could also be a custom defined name. UserId isn't clear enough.
| return name.substring(start, end); | ||
| } | ||
|
|
||
| String formatedTime(int value) { |
There was a problem hiding this comment.
Shouldn't we just reuse existing time for this instead? Rather than building our own?
Also okay with just putting using time up as a ticket and link that here with a FIXME, replace with http...
There was a problem hiding this comment.
I haven't checked this package but I initially used duration and it was not correctly parsing the seconds into hours (the calculated was 5 hours less than actual). So I came up with own custom solution. I can open ticket for it if we want to use that instead of custom function.
| @@ -0,0 +1,32 @@ | |||
| // import 'package:flutter/material.dart'; | |||
There was a problem hiding this comment.
Files with just outcommented code also don't belong into the repo ;)
| leading: FutureBuilder<Uint8List>( | ||
| future: room.avatar().then((fb) => fb.asTypedList()), | ||
| // a previously-obtained Future<String> or null | ||
| builder: (BuildContext context, AsyncSnapshot<List<int>> snapshot) { | ||
| if (snapshot.hasData) { | ||
| return CircleAvatar( | ||
| radius: 25, | ||
| backgroundImage: | ||
| MemoryImage(Uint8List.fromList(snapshot.requireData)), | ||
| ); | ||
| } else { | ||
| return CircleAvatar( | ||
| radius: 25, | ||
| backgroundColor: Colors.grey[700], | ||
| child: SvgPicture.asset('assets/images/people.svg'), | ||
| ); | ||
| } | ||
| }, | ||
| ), |
There was a problem hiding this comment.
the .avatar() is a very common pattern. Can we outsource that into a stand-along reusable component with configurabel radius, please?
There was a problem hiding this comment.
Yes that would be good, I can make custom widget for it. Good catch!
| padding: const EdgeInsets.only(right: 10), | ||
| child: CircleAvatar( | ||
| radius: 15, | ||
| child: Text(getNameFromId(userId)[0].toUpperCase()), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
We should reuse the (then new) avatar-component here. The user-name-to-icon should only be a fallback.
No description provided.