zulip-flutter icon indicating copy to clipboard operation
zulip-flutter copied to clipboard

Compose box redesign

Open PIG208 opened this issue 1 year ago • 11 comments

Fixes: #915

PIG208 avatar Sep 04 '24 23:09 PIG208

(getting the screenshots ready)

PIG208 avatar Sep 07 '24 00:09 PIG208

The design specifies a 8px high padding before the message content:

image

The content can be scrolled past the padding, then clipped at the top border of the compose box:

image

This turned out to be tricky to implement, because InputDecorator's contentPadding also pads the scrollable view, and the content cannot be scrolled past the padding, leaving a gap:

with padding without padding
image Screenshot from 2024-09-07 00-50-07

A previous workaround was to fully vertically expand TextField by wrapping it within a SingleChildScrollView (implemented in e16251febac9b000c50969845025bc4a1b54e67a), which does not handle the case that the cursor gets scrolled out of view when editing:

Example

image

The latest workaround disables clipping on the TextField and wraps it with a ClipRect, whose size is determined by a ConstrainedBox under our control.

PIG208 avatar Sep 07 '24 04:09 PIG208

I think I have found a true simple solution to this: set TextField.clipBehavior to Clip.none, set InputDecoration.contentPadding and wrap the TextField with a ClipRect under our control. This seems to work perfectly without the cursor issue.

PIG208 avatar Sep 10 '24 20:09 PIG208

Preview
light dark
1000014499 1000014506
1000014500 1000014501
1000014503 1000014502
1000014504 1000014505
1000014508 1000014507
1000014509 1000014510
1000014512 1000014511

PIG208 avatar Sep 10 '24 21:09 PIG208

Updated the PR with the screenshots. Ready for @chrisbobbe and @alya to review!

PIG208 avatar Sep 10 '24 21:09 PIG208

I haven't compared carefully vs. the designs in Figma, but the screenshots look nice!

alya avatar Sep 17 '24 16:09 alya

Thanks for the previous comments @chrisbobbe! This is ready for review now.

PIG208 avatar Sep 23 '24 23:09 PIG208

This is ready for review now (forgot to comment on that).

PIG208 avatar Sep 26 '24 01:09 PIG208

Extracted InsetShadowBox so that #853 can reuse this.

PIG208 avatar Oct 08 '24 00:10 PIG208

~~Will push an update here soon.~~

Ready for review!

PIG208 avatar Oct 08 '24 20:10 PIG208

Cool, reviewing now.

chrisbobbe avatar Oct 10 '24 21:10 chrisbobbe

Thanks. The PR is ready for review again!

PIG208 avatar Oct 23 '24 20:10 PIG208

Thanks for the review! I have updated the PR.

The last three commits are for the proof-of-concept tests for the content input scaling feature; they are optional:

  • 3e282e4b wip; compose_box content input text scaling test
  • 81b27f55 compose_box test: Set topic and content through a helper (cherry-picked from #1033)
  • 93d5f999 compose_box test: Use a layout similar to the message list page (#1034)

Not sure if this aspect of the UI requires its own set of tests verifying the maximum number of lines to display. It basically verifies that the max height sticks to a certain number, and that the maximum number of visible lines matches where we expected from the figma design.

PIG208 avatar Nov 01 '24 05:11 PIG208

Thanks for the review! I have updated the PR.

There is another revision on top of this that I didn't push, because it reorganizes the commits (even though the diff with all commits combined is identical to this one), which might make verifying the update a bit more difficult.

PIG208 avatar Nov 08 '24 23:11 PIG208

tools/check is failing at this commit:

compose_box: Use new icons from the redesign
00:19 +1525 ~166: /Users/chrisbobbe/dev/zulip-flutter/test/widgets/compose_box_test.dart: ComposeBox typing notices hitting send button sends a "typing stopped" notice
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown running a test:
The finder "Found 0 widgets with icon "IconData(U+0E571)": []" (used in a call to "tap()") could not
find any matching widgets.

When the exception was thrown, this was the stack:
#0      WidgetController._getElementPoint (package:flutter_test/src/controller.dart:1895:7)
#1      WidgetController.getCenter (package:flutter_test/src/controller.dart:1795:12)
#2      WidgetController.tap (package:flutter_test/src/controller.dart:1043:18)
#3      main.<anonymous closure>.<anonymous closure> (file:///Users/chrisbobbe/dev/zulip-flutter/test/widgets/compose_box_test.dart:281:20)
<asynchronous suspension>
#4      testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:189:15)
<asynchronous suspension>
#5      TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1027:5)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)

The test description was:
  hitting send button sends a "typing stopped" notice
════════════════════════════════════════════════════════════════════════════════════════════════════
00:19 +1525 ~166 -1: /Users/chrisbobbe/dev/zulip-flutter/test/widgets/compose_box_test.dart: ComposeBox typing notices hitting send button sends a "typing stopped" notice [E]
  Test failed. See exception logs above.
  The test description was: hitting send button sends a "typing stopped" notice

chrisbobbe avatar Nov 12 '24 01:11 chrisbobbe

Pushed to fix this. Thanks!

PIG208 avatar Nov 12 '24 03:11 PIG208

Added:

  • eaed8fba emoji: Use withFadedAlpha for highlightColor
Screenshots (highlighted reactions)
before after
Screenshot from 2024-11-12 11-33-36 Screenshot from 2024-11-12 11-34-03
  • b60f13c6 color: Add ColorExtension.withFadedAlpha

There are some remaining callers of withValues:

Details
$ git grep withValues
lib/widgets/channel_colors.dart:          .withValues(alpha: 0.3),
lib/widgets/channel_colors.dart:          .withValues(alpha: 0.3),
lib/widgets/color.dart:    return withValues(alpha: a * factor);
lib/widgets/content.dart:      colorThematicBreak: const HSLColor.fromAHSL(1, 0, 0, .87).toColor().withValues(alpha: 0.2),
lib/widgets/emoji_reaction.dart:      borderSelected: Colors.black.withValues(alpha: 0.45),
lib/widgets/emoji_reaction.dart:      borderUnselected: Colors.black.withValues(alpha: 0.05),
lib/widgets/emoji_reaction.dart:      bgSelected: Colors.black.withValues(alpha: 0.8),
lib/widgets/emoji_reaction.dart:      bgUnselected: Colors.black.withValues(alpha: 0.3),
lib/widgets/emoji_reaction.dart:      borderSelected: Colors.white.withValues(alpha: 0.75),
lib/widgets/emoji_reaction.dart:      borderUnselected: Colors.white.withValues(alpha: 0.15),
lib/widgets/emoji_reaction.dart:      textSelected: Colors.white.withValues(alpha: 0.85),
lib/widgets/emoji_reaction.dart:      textUnselected: Colors.white.withValues(alpha: 0.75),
lib/widgets/inset_shadow.dart:      colors: [color, color.withValues(alpha: 0)]));
lib/widgets/lightbox.dart:    final appBarBackgroundColor = Colors.grey.shade900.withValues(alpha: 0.87);
lib/widgets/message_list.dart:      streamRecipientHeaderChevronRight: Colors.black.withValues(alpha: 0.3),
lib/widgets/message_list.dart:      unreadMarkerGap: Colors.white.withValues(alpha: 0.6),
lib/widgets/message_list.dart:      streamRecipientHeaderChevronRight: Colors.white.withValues(alpha: 0.3),
lib/widgets/theme.dart:      bgCounterUnread: const Color(0xff666699).withValues(alpha: 0.15),
lib/widgets/theme.dart:      borderBar: Colors.black.withValues(alpha: 0.2),
lib/widgets/theme.dart:      editorButtonPressedBg: Colors.black.withValues(alpha: 0.06),
lib/widgets/theme.dart:      groupDmConversationIcon: Colors.black.withValues(alpha: 0.5),
lib/widgets/theme.dart:      modalBarrierColor: const Color(0xff000000).withValues(alpha: 0.3),
lib/widgets/theme.dart:      unreadCountBadgeTextForChannel: Colors.black.withValues(alpha: 0.9),
lib/widgets/theme.dart:      bgCounterUnread: const Color(0xff666699).withValues(alpha: 0.37),
lib/widgets/theme.dart:      borderBar: Colors.black.withValues(alpha: 0.5),
lib/widgets/theme.dart:      contextMenuCancelText: const Color(0xffffffff).withValues(alpha: 0.75),
lib/widgets/theme.dart:      editorButtonPressedBg: Colors.white.withValues(alpha: 0.06),
lib/widgets/theme.dart:      labelCounterUnread: const Color(0xffffffff).withValues(alpha: 0.7),
lib/widgets/theme.dart:      labelMenuButton: const Color(0xffffffff).withValues(alpha: 0.85),
lib/widgets/theme.dart:      textInput: const Color(0xffffffff).withValues(alpha: 0.9),
lib/widgets/theme.dart:      groupDmConversationIcon: Colors.white.withValues(alpha: 0.5),
lib/widgets/theme.dart:      modalBarrierColor: const Color(0xff000000).withValues(alpha: 0.5),
lib/widgets/theme.dart:      unreadCountBadgeTextForChannel: Colors.white.withValues(alpha: 0.9),
test/widgets/color_test.dart:        .isSameColorAs(color.withValues(alpha: 0.25));
test/widgets/color_test.dart:        .isSameColorAs(color.withValues(alpha: 0.5));

Most of them, other than the ones in lib/widgets/channel_colors.dart are from themes defining the colors. The obvious ones are handled in the new commits, and we can do a full sweep in a follow-up PR.

I also noticed that the text scale tests stop being reliable when the text scale factor is lower than or equal to 0.5, reporting that we get less than 8 fully or partially visible lines. However, that doesn't happen when I run flutter run test/widgets/compose_box_test.dart, which is puzzling.

PIG208 avatar Nov 12 '24 17:11 PIG208

I also noticed that the text scale tests stop being reliable when the text scale factor is lower than or equal to 0.5

Odd. But iOS at least doesn't let you choose a factor that small. 🤷‍♂️ We've written down some of iOS's options in test/widgets/text_test.dart:

// From trying the options on an iPhone 13 Pro running iOS 16.6.1:
const kTextScaleFactors = <double>[
  0.8235, // smallest
  1,
  1.3529, // largest without using the "Larger Accessibility Sizes" setting
  3.1176, // largest
];

chrisbobbe avatar Nov 12 '24 22:11 chrisbobbe

Thanks! Updated the PR.

PIG208 avatar Nov 13 '24 01:11 PIG208

I have pushed a change reorganizing the commits for better readability. This is effectively an NFC change overall — no functionality change after reordering.

Before 66d55db6 theme: Update borderBar to match Figma design 6dd42643 compose_box: Use new icons from the redesign f52104a3 compose_box: Use new send button color, shape and icon b13840c3 compose_box [nfc]: Refactor away shared iconTheme override d18ec182 fix link; compose_box: Support new compose box layout d5fa7f93 compose_box: Support redesigned button feedback 23ef514f color [nfc]: Add ColorExtension.withFadedAlpha a29e69b5 emoji: Use withFadedAlpha for highlightColor 00386935 compose_box: Implement topic input redesign 6df7931e compose_box: Implement content input redesign f8f97dd7 optional; compose_box content input text scaling test

After prep changes 13ab190c color [nfc]: Add ColorExtension.withFadedAlpha ef6cf91b emoji: Use withFadedAlpha for highlightColor ce761fdf theme: Update borderBar to match Figma design a3da9918 compose_box [nfc]: Refactor away shared iconTheme override 2780d25f compose_box: Use new icons from the redesign main redesign changes 17372299 fix link; compose_box: Support new compose box layout 455a28ec compose_box: Use new send button color, shape and icon 70376681 compose_box: Support redesigned button feedback 61904987 compose_box: Implement input redesign cbac604e optional; compose_box content input text scaling test

Changes

  • Moved 23ef514f color [nfc]: Add ColorExtension.withFadedAlpha and a29e69b5 emoji: Use withFadedAlpha for highlightColor to the very front of the sequence. They no longer touch the colors that use withValues in lib/widgets/compose_box.dart, leaving them to the later commits that implement the UI changes.

  • Grouped the lighter commits together as prep changes: 17372299 fix link; compose_box: Support new compose box layout 455a28ec compose_box: Use new send button color, shape and icon 70376681 compose_box: Support redesigned button feedback 61904987 compose_box: Implement input redesign cbac604e (HEAD -> main, me/dev-compose) optional; compose_box content input text scaling test These commits barely requires checking the Figma redesign, other than confirming the icons

  • Moved f52104a3 compose_box: Use new send button color, shape and icon after d18ec182 fix link; compose_box: Support new compose box layout so that the order is more natural, simplifying the diffs for changes relevant to the send button.

  • Combined 00386935 compose_box: Implement topic input redesign and 6df7931e compose_box: Implement content input redesign into 61904987 compose_box: Implement input redesign

PIG208 avatar Nov 13 '24 04:11 PIG208

Cool, thanks for the update! Reviewing again now.

chrisbobbe avatar Nov 13 '24 22:11 chrisbobbe

Moving this to integration review since I'm about to be on vacation.

chrisbobbe avatar Nov 14 '24 02:11 chrisbobbe

Thanks for the review! I have updated the PR.

PIG208 avatar Nov 15 '24 03:11 PIG208

One commit-message nit: use "compose" for the prefix rather than "compose_box"

Ah, I see we raced and you've already made that change :slightly_smiling_face:

gnprice avatar Nov 15 '24 21:11 gnprice

Thanks! Updated the PR.

PIG208 avatar Nov 15 '24 22:11 PIG208

Moved the links to the paragraph they each correspond to, and added the comment on ClipRect (the related paragraph is still kept in the commit, in case the reader is looking back at this for a more verbose explanation on this design's origin).

PIG208 avatar Nov 15 '24 22:11 PIG208

Looks good — merging. Thanks again for all your work on this!

gnprice avatar Nov 16 '24 00:11 gnprice