Improvements on messaging UI, and fix on minor bugs on NestedScroll #154

Crdzbird · 2021-06-15T20:32:41Z

WIP there are gonna be more changes on the UI.

Reduced the nested code. Improve the building time and performance.

- Emojis display overflow or actions are overwritten

Resolved issue of scroll overlays pager on sublistviews

cosmicespresso

Left some comments but works fine for me!

lib/messaging/conversation.dart

cosmicespresso · 2021-06-15T21:12:47Z

lib/messaging/widgets/messaging_emoji_picker.dart

+  const MessagingEmojiPicker({
+    required this.showEmojis,
+    required this.emptySuggestions,
+    this.height = 200,


this is totally fine for now, ideally we'd use some relative measure (% of viewport height)

The height is using a percentage of the total height of the screen, and the width was removed because by default takes all the available space horizontally.

pubspec.yaml

cosmicespresso · 2021-06-15T21:18:51Z

lib/ui/home.dart

+                        ..onCancel = _handleDragCancel;
+                    })
+                  },
+                  behavior: HitTestBehavior.opaque,


in conversation.dart I switched HitTestBehavior to translucent since the emoji reactions were not picked up very well, is it a problem?

In my case i was receiving an overflow, and after that, for some reason when i try to scroll horizontally on the emoji response bubble, the movement was detected as a page transition, the workaround was to use a wrapper inside the pager and pass a controller to the desired widget, and once everything was done the ui recognize better the transitions

Regarding the emoji reactions, I don't think we want to be scrolling horizontally forever. The idea was that we'd have 5 main ones (which I had added) and then a "...." icon that would trigger the emoji keyboard (see below), where a user could pick any emoji.

Im gonna add the functionality to open the emoji container, however im thinking into still leaving the option to scroll, mainly because if one user decides to open the app on a phone with little dimensions, it can lead to a blank space. The alternative it can be to set those emojis in a Flexible space so it can adapt without scrolling @Kallirroi please tell me what do you think

I think we'd need to test out both options and involve @Derekf5 - the infinite horizontal scrolling in such a small container might be a bit annoying for the user.

I think you are 100% right things need to be adapted for smaller screens, but we will potentially need to rethink how that entire popup menu behaves in that case, as opposed to just the reaction emojis.

Sorry, I commented about this outside of this thread, but having both horizontal scroll and ellipses seems redundant. The ellipses provide a mechanism for handling overflow, which I think is more user friendly than horizontal scrolling.

- Created custom emoji selector. - Improve on rendering ui

Crdzbird · 2021-06-17T13:41:57Z

this is the final result following the ui style @Kallirroi also the implementation of the dots as emoji response is implemented on the recent commits.

Crdzbird · 2021-06-17T13:49:37Z

Im still struggling about how to handle the widget transition between permission request and denied, without having to do more changes on the UI 🤔 for today im hopping to finish all the changes

oxtoacart · 2021-06-17T14:17:34Z

I know this is still a WIP, but I see a couple of issues:

the message bar (smiley, text input, buttons) is not vertically centered

I don't like the horizontal scrolling on the reaction emojis. IIUC the problem you're trying to solve is overflow on smaller screens. The design already includes a button with ellipses (...), which already provides a mechanism for handling overflow. The way that I think it should work is that we display as many emojis as the space allows and use the ellipses overflow for the rest.

oxtoacart · 2021-06-17T14:32:48Z

@Crdzbird Is there a ticket(s) for the problem(s) that you're trying to solve? I'm a little concerned that we might be spending time on something that's not ticketed and not assigned to this sprint.

Crdzbird · 2021-06-17T14:35:49Z

Yep

https://github.com/orgs/getlantern/projects/9#card-63082934

https://github.com/orgs/getlantern/projects/9#card-63186904

Crdzbird · 2021-06-17T14:39:35Z

@oxtoacart the thing with the reactions, is that when i start on the first try without the dots, they already took all the space of the screen,and already have some overflow by 7pixels on my device, when the dots where added then that overflow increments, so I decided to implement the scrolling interaction, also because the same reaction list was already inside a list view so I only add the change in case of the emojis reactions are too big to render on more devices

Crdzbird · 2021-06-17T14:40:56Z

Also if I used a flex then the reactions shrinks by a lot and it's kinda of uncomfortable to tap on the desired reaction reduced

oxtoacart · 2021-06-17T14:45:23Z

@oxtoacart the thing with the reactions, is that when i start on the first try without the dots, they already took all the space of the screen,and already have some overflow by 7pixels on my device, when the dots where added then that overflow increments, so I decided to implement the scrolling interaction, also because the same reaction list was already inside a list view so I only add the change in case of the emojis reactions are too big to render on more devices

So I don't know how to implement it, but the best behavior would to display a variable number of emojis based on the available space, and always display the ellipses.

cosmicespresso · 2021-06-17T18:07:05Z

@Crdzbird I just noticed that the horizontal dragging between screens messes up with the Voice Recorder (which needs to be swiped left to cancel). Take a look if you want! 🙂

- Created functionality when user tap on dots to add custom reaction. - Removed horizontal behaviour and replaced with a flexible manager. - Fix permission checker and reduced code base

Crdzbird · 2021-06-17T21:03:57Z

lib/messaging/conversation.dart

    if (_recording) {
      return;
    }
-
-    model.startRecordingVoiceMemo().then((value) {
+    _hasPermission = await model.startRecordingVoiceMemo();


I have to change this into a fully async, previously this will continue the process leading to a render error, and when the user deny the permission the app gets confused and render the recording action

Crdzbird · 2021-06-17T21:05:22Z

lib/messaging/conversation.dart

+                      dismissKeyboard();
+                    }
+                  },
+                ),


To handle the events of drag, and more, it was necessary to add custom functions as required params, also for the emoji reaction with the 3 dots, a global function was declared and linked on the reactions.dart

Crdzbird · 2021-06-17T21:06:11Z

lib/messaging/conversation.dart

          if (_recording)
            VoiceRecorder(
+              onSwipeLeft: () => setState(() => _recording = false),


This was needed to cancel the audio recording when the gesture detect that x is <= 200

Crdzbird · 2021-06-17T21:06:54Z

lib/messaging/conversation.dart

+            onEmojiTap: (showEmoji, messageSelected) => setState(() {
+              _emojiShowing = true;
+              _customEmojiResponse = true;
+              _storedMessage = messageSelected;


This variable stores the message in which we desire to attach a custom emoji (not available from the 5 reactions by default)

Crdzbird · 2021-06-17T21:07:11Z

lib/messaging/conversation.dart

            onReply: (_message) {
              setState(() {
                _isReplying = true;
                _quotedMessage = _message;
-                showKeyboard(); // TODO: this is clashing with Navigator.pop(context);
+                showKeyboard();


Fixed issue with navigator

Crdzbird · 2021-06-17T21:07:56Z

lib/messaging/conversation.dart

-            )),
-      ),
-    );
-  }


All of this was moved to separated widget classes, to have a more absolute control

Crdzbird · 2021-06-17T21:08:33Z

lib/messaging/messaging.dart

@@ -40,7 +40,8 @@ class _MessagesTabState extends State<MessagesTab>
              break;
            case '/conversation':
              builder = (BuildContext context) => Conversation(
-                  settings.arguments ?? widget._initialRouteArguments);
+                    settings.arguments ?? widget._initialRouteArguments,


removed previous handler when the reactions were wrapped inside a list view with scroll

cosmicespresso

Generally looks 👍 - I added two comments which I think are not blockers for merging this.
Some documentation feedback:

do you mind providing a summary of the changes instead of "WIP there are gonna be more changes on the UI." in the opening comment? 😇
I had opened this https://github.com/getlantern/android-lantern/issues/142 and this https://github.com/getlantern/android-lantern/issues/139 tickets recently, if you think the changes address either then connect them to this PR.

cosmicespresso · 2021-06-21T17:12:29Z

lib/messaging/widgets/reactions.dart

+                      shape: BoxShape.circle,
+                      color: Colors.white,
+                      boxShadow: [
+                        BoxShadow(


not sure we should be adding the shadow here, personally I'd just skip it - otherwise looks and works 💯

hhmmm... the other alternative is to use an ElevatedButton and manage the button states, (uploaded on latest commit)

cosmicespresso · 2021-06-21T17:15:23Z

lib/messaging/widgets/voice_recorder.dart

+      clipBehavior: Clip.none,
+      fit: StackFit.passthrough,
+      children: [
+        isRecording


Recording is pretty broken for me:

there is an uncaught exception with the scrollListener: _AssertionError ('package:scrollable_positioned_list/src/scrollable_positioned_list.dart': Failed assertion: line 226 pos 12: '_scrollableListState != null': is not true.)

swiping left to cancel swipes entire screen to the VPN view

I suggest we create a new ticket for just focusing on the Voice Recorder separately, since it requires a lot of attention.

@Kallirroi the error as I found is related to this segment of the code:

await _scrollController.scrollTo( index: 00, duration: const Duration(seconds: 1), curve: Curves.easeInOutCubic);

at the moment I comment everything related to that and works fine

cosmicespresso · 2021-06-21T17:16:39Z

lib/utils/custom_pan_gesture_recognizer.dart

+
+  @override
+  String get debugDescription => 'CustomPanRecognizer';
+}


Nice, this sort of refactoring is very helpful for me to read 👍

oxtoacart · 2021-06-21T19:37:53Z

Alignment on the message box still looks wrong. It should be centered vertically, correct?

oxtoacart · 2021-06-21T19:40:26Z

The record button doesn't align correctly to the right of the screen on my Huawei Mate 9.

oxtoacart · 2021-06-21T19:40:44Z

Voice recordings aren't coming through

Crdzbird · 2021-06-21T19:43:38Z

Alignment on the message box still looks wrong. It should be centered vertically, correct?

weird... on the test device, the alignment is always centered, Im gonna see if i can display that missalign on an emulated device 🤔

oxtoacart · 2021-06-21T19:46:07Z

Voice recordings aren't coming through

Looks like this problem is not new to this branch.

Crdzbird · 2021-06-21T19:48:12Z

Voice recordings aren't coming through

The voice functionality isn't part of this PR, the only parts that I was working were related to the permission error, the swipe that isn't recognized and instead change the page and finally the reactions with the dots @oxtoacart that is gonna go on a different PR to handle the reproduction 👀

now the reactions states are working based on the UI Guideline in figma

…o crdzbird/permission_handler

- Line 117 of conversation.dart.

oxtoacart · 2021-06-21T23:02:38Z

Alignment on the message box still looks wrong. It should be centered vertically, correct?

weird... on the test device, the alignment is always centered, Im gonna see if i can display that missalign on an emulated device 🤔

If it helps any, the alignment is fine on ox/messaging.

Removed everything related to page change. Replaced CustomWidget wit a GestureDetector, due to now the pager widget is unable to change between pages when the user swipe. - Improved performance and fix issue related when user cancel the recording. This was causing a buffer overflow on the audio recorder native and leading to an unexpected end of the app.

Crdzbird · 2021-06-23T02:03:36Z

lib/messaging/widgets/countdown_timer.dart

+import 'package:flutter/material.dart';
+import 'package:stop_watch_timer/stop_watch_timer.dart';
+
+class CountdownTimer extends StatelessWidget {


This was separated as an independent widget due to only be useful just to display a numeric countdown.

Crdzbird · 2021-06-23T02:05:29Z

lib/messaging/widgets/message_types/status_row.dart

@@ -40,42 +40,37 @@ class StatusRowState extends State<StatusRow> {
          return Container(
            child: Opacity(
              opacity: 0.8,
-              child: Flex(
+              child: Wrap(


The wrap was needed because when the bubble options appears, the date of the message below creates an empty gap leading to a render problem

Crdzbird · 2021-06-23T02:06:04Z

lib/messaging/widgets/message_types/status_row.dart

                    ),
                  ),
                  if (statusIcon != null)
-                    Flexible(


The wrap handle everything related to size for all the childrens, so the flexible can be removed

Crdzbird · 2021-06-23T02:08:08Z

lib/messaging/widgets/reactions.dart

+              (e) => Flexible(
+                child: Container(
+                  margin: const EdgeInsets.symmetric(horizontal: 4.0),
+                  child: TextButton(


By suggestion the previous ui of the reactions was replaced, and now is using a TextButton, we can gain access to a better button state handler and change the material state when the button is onHold, adding a nice elevation effect only to the pressed widget.

Crdzbird · 2021-06-23T02:09:04Z

lib/ui/home.dart

@@ -133,6 +132,7 @@ class _HomePageState extends State<HomePage> {
                body: PageView(
                  onPageChanged: onPageChange,
                  controller: _pageController,
+                  physics: const NeverScrollableScrollPhysics(),


Added an unscrollable physic due to the pager now being static

Crdzbird · 2021-06-23T02:11:12Z

lib/utils/custom_pan_gesture_recognizer.dart

+import 'package:flutter/material.dart';
+
+/// Handle a native gesture detection without the need to have more than one listener
+class ForcedPanDetector extends StatelessWidget {


I don't know if we are gonna need this for a future, this widget lets you handle an independent tap gesture without having to worry about any parent widget with a gesture detector recognize the tap as his own instead of the children.

oxtoacart · 2021-06-23T16:55:57Z

LGTM!

Crdzbird added 5 commits June 14, 2021 16:39

Refactoring of Messaging UI

6106e81

changes on reactions builder

61ce577

Reduced the nested code. Improve the building time and performance.

Removed Builder and moved into a independent widget

d2d9cb3

Fix issue on messaging emoji picker

b493345

- Emojis display overflow or actions are overwritten

Scroll fix

afc3af2

Resolved issue of scroll overlays pager on sublistviews

Crdzbird requested review from oxtoacart and cosmicespresso June 15, 2021 20:32

Removed unnecessary widget

8e14a05

cosmicespresso reviewed Jun 15, 2021

View reviewed changes

Crdzbird added 3 commits June 17, 2021 07:32

Working on functionalities

abe8ff9

- Created custom emoji selector. - Improve on rendering ui

code reduction on keyboard

0c503fd

Temporary return to old message bar (with new functionality)

2820578

Crdzbird added 2 commits June 17, 2021 13:29

Fix on ui and added functionality on dotted

f14a2da

- Created functionality when user tap on dots to add custom reaction. - Removed horizontal behaviour and replaced with a flexible manager. - Fix permission checker and reduced code base

Adding gesture recognition and base handler

81a4a17

Crdzbird commented Jun 17, 2021

View reviewed changes

lib/messaging/conversation.dart

)),

),

);

}

Copy link

Author

Crdzbird Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this was moved to separated widget classes, to have a more absolute control

Crdzbird commented Jun 17, 2021

View reviewed changes

cosmicespresso reviewed Jun 21, 2021

View reviewed changes

tiny UI fix

bc055b8

fix reaction emoji alignment

44f8e56

Crdzbird added 5 commits June 21, 2021 14:29

changed ui of reaction list

de36c9f

now the reactions states are working based on the UI Guideline in figma

Fix of position on recording red circle, height fix

15c9c00

Merge remote-tracking branch 'origin/crdzbird/permission_handler' int…

bab6f7b

…o crdzbird/permission_handler

Reduced pixel gap to stop recording.

5c07bc4

Commented segment of code that throws an exception

03c1d1f

- Line 117 of conversation.dart.

Base automatically changed from kr/contact_scanning to ox/messaging June 22, 2021 16:09

Crdzbird added 2 commits June 22, 2021 19:59

Removed unneded function

95482ba

Crdzbird requested a review from cosmicespresso June 23, 2021 02:02

Crdzbird commented Jun 23, 2021

View reviewed changes

Crdzbird added 2 commits June 23, 2021 09:46

Removed debug prints

1b3579d

linter

15d8800

oxtoacart merged commit 1edc354 into ox/messaging Jun 23, 2021

oxtoacart deleted the crdzbird/permission_handler branch June 23, 2021 16:56

Improvements on messaging UI, and fix on minor bugs on NestedScroll #154

Improvements on messaging UI, and fix on minor bugs on NestedScroll #154

Conversation

Crdzbird commented Jun 15, 2021 • edited Loading

cosmicespresso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crdzbird Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crdzbird commented Jun 17, 2021 • edited Loading

Crdzbird commented Jun 17, 2021

oxtoacart commented Jun 17, 2021

oxtoacart commented Jun 17, 2021

Crdzbird commented Jun 17, 2021

Crdzbird commented Jun 17, 2021

Crdzbird commented Jun 17, 2021

oxtoacart commented Jun 17, 2021

cosmicespresso commented Jun 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicespresso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxtoacart commented Jun 21, 2021

oxtoacart commented Jun 21, 2021

oxtoacart commented Jun 21, 2021

Crdzbird commented Jun 21, 2021 • edited Loading

oxtoacart commented Jun 21, 2021

Crdzbird commented Jun 21, 2021 • edited Loading

oxtoacart commented Jun 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crdzbird Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxtoacart commented Jun 23, 2021

Crdzbird commented Jun 15, 2021 •

edited

Loading

Crdzbird Jun 16, 2021 •

edited

Loading

Crdzbird commented Jun 17, 2021 •

edited

Loading

Crdzbird commented Jun 21, 2021 •

edited

Loading

Crdzbird commented Jun 21, 2021 •

edited

Loading

Crdzbird Jun 23, 2021 •

edited

Loading