Compose box redesign #928
Compose box redesign #928
Conversation
PIG208
force-pushed
the
pr-compose
branch
5 times, most recently
from
c17497f
to
a566c67
Compare
|
(getting the screenshots ready) |
PIG208
force-pushed
the
pr-compose
branch
3 times, most recently
from
c96e62c
to
525240a
Compare
|
The design specifies a 8px high padding before the message content:
The content can be scrolled past the padding, then clipped at the top border of the compose box:
This turned out to be tricky to implement, because
A previous workaround was to fully vertically expand Example
|
PIG208
force-pushed
the
pr-compose
branch
2 times, most recently
from
9f9fb43
to
340a5ce
Compare
|
I think I have found a true simple solution to this: set |
Preview
|
|
Updated the PR with the screenshots. Ready for @chrisbobbe and @alya to review! |
|
I haven't compared carefully vs. the designs in Figma, but the screenshots look nice! |
PIG208
force-pushed
the
pr-compose
branch
2 times, most recently
from
c8f40f4
to
d95273c
Compare
The icons are mainly exported from the Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=544-22131&node-type=CANVAS&t=hNeqhPGojoFpTJnp-0 Except image.svg, which was downloaded from the original source of the icon (https://lucide.dev/icons/image). Due to the limitation of the icon font, image.svg and send.svg have been adjusted with a tool that converts SVG strokes to fills: https://www.npmjs.com/package/oslllo-svg-fixer See also: - https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Icon.20not.20rendering.20correctly/near/1936793 Signed-off-by: Zixuan James Li <[email protected]>
DesignVariables.icon gets used for the first time in this commit, and its value has been updated to match the current design. We removed the `ButtonStyle` for the send button that was added in # 399, which was irrelevant to the new design. See also: - https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3954-13395 - https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3862-14350
Once we have the new design for all buttons, we can proceed to disable splash effect globally. For redesign projects like this, we adapt to splashless buttons with a local theme. See also: - https://github.com/zulip/zulip-flutter/pull/ 853#discussion_r1720334991 Signed-off-by: Zixuan James Li <[email protected]>
While `_composeButtonHeight` isn't used elsewhere, we place it next to
`_composeButtonWidth` for better discoverability.
We updated a comment to explain why we need to set `contentPadding`
even if it doesn't seem to be necessary.
See also:
- `InputDecoration.contentPadding`, which documents the nuances of
the default value of `contentPadding`.
- https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3954-13395
- https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3862-14350
Signed-off-by: Zixuan James Li <[email protected]>
`Spoiler` has a similar bottom underline implementation. We could have used `UnderlineInputBorder` on `InputDecoration`, but that also comes with other input state styles (e.g.: focused, disabled, etc.). See also: - https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3954-13395 - https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3862-14350 Signed-off-by: Zixuan James Li <[email protected]>
The shadow is always present, but the overlay is only visible when there is text under it. This only happens when the TextField is long enough to be scrollable. See also: - https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3954-13395 - https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3862-14350 Fixes: zulip#915 Signed-off-by: Zixuan James Li <[email protected]>
| @@ -110,7 +110,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> { | |||
| bgCounterUnread: const Color(0xff666699).withValues(alpha: 0.15), | |||
| bgTopBar: const Color(0xfff5f5f5), | |||
| borderBar: const Color(0x33000000), | |||
| icon: const Color(0xff666699), | |||
| icon: const Color(0xff6159e1), | |||
There was a problem hiding this comment.
FIX LINK compose_box: Implement compose box buttons redesign
DesignVariables.icon gets used for the first time in this commit,
and its value has been updated to match the current design.
We removed the `ButtonStyle` for the send button that was added
in # 399, which was irrelevant to the new design.
See also:
- https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3954-13395
- https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3862-14350
There's an existing use of DesignVariables.icon: in our ThemeData.appBarTheme.actionsIconTheme.color. This shows up in a list if I Cmd-click on the icon field declaration in Android Studio.
| ), | ||
| color: foregroundColor, | ||
| color: _hasValidationErrors | ||
| // TODO(design): need send button color when disabled |
There was a problem hiding this comment.
From the Figma, it looks like designVariables.icon with 50% opacity is right…except the 50% opacity is actually meant to apply to basically all of the compose box, not just the send button, right:
| style: const ButtonStyle( | ||
| // Match the height of the content input. | ||
| minimumSize: WidgetStatePropertyAll(Size.square(_sendButtonSize)), | ||
| // With the default of [MaterialTapTargetSize.padded], not just the | ||
| // tap target but the visual button would get padded to 48px square. | ||
| // It would be nice if the tap target extended invisibly out from the | ||
| // button, to make a 48px square, but that's not the behavior we get. | ||
| tapTargetSize: MaterialTapTargetSize.shrinkWrap, | ||
| ), | ||
| color: foregroundColor, |
There was a problem hiding this comment.
FIX LINK compose_box: Implement compose box buttons redesign
In this commit, it looks like we're no longer trying to match the send button's height to the (min) height of the content input. That's fine, but let's remove all the involved code together in this commit. In particular, at this commit, the code isn't clear about what _sendButtonSize is for, and also it doesn't correspond to the actual size of the send button.
The first thing I'd try is to just remove the minHeight that uses _sendButtonSize, and accept Material's default min height for the input. Then remove _sendButtonSize, also in this commit.
| labelCounterUnread: const Color(0xff222222), | ||
| labelEdited: const HSLColor.fromAHSL(0.35, 0, 0, 0).toColor(), | ||
| labelMenuButton: const Color(0xff222222), | ||
| mainBackground: const Color(0xfff0f0f0), | ||
| title: const Color(0xff1a1a1a), | ||
| bgComposeBox: const Color(0xffffffff), |
There was a problem hiding this comment.
The Figma color variable is called composeBoxBg; let's match that. I don't love that the list of Figma color variables isn't internally consistent (sometimes putting "bg" at the beginning, sometimes at the end), but at least if we match each of our variables to the corresponding one in Figma (I guess modulo camel case vs. snake case) than we'll be making it easier on ourselves to check the implementation against the design.
| final Color bgComposeBox; | ||
| final Color textInput; | ||
| final Color foreground; |
There was a problem hiding this comment.
It's good to put these here, in the section for things that are actually design variables in the Figma. But they were in alphabetical order before; can we keep that, if there's not a reason not to?
| @@ -867,18 +867,16 @@ class _ComposeBoxContainer extends StatelessWidget { | |||
|
|
|||
| @override | |||
| Widget build(BuildContext context) { | |||
| ColorScheme colorScheme = Theme.of(context).colorScheme; | |||
| final designVariables = DesignVariables.of(context); | |||
There was a problem hiding this comment.
compose_box: Use new container background
The commit message makes it sound like the only visible change is in how a background looks, but that turns out not to be true, right? It looks like the foreground elements (buttons and inputs) would be moved around in this commit, looking at the change to the SafeArea and the removed Padding.
We could split the layout change out into its own commit. But actually, looking at this and other commits in the PR, I wonder if it's a case where lumping together is more helpful than splitting apart. Lumping can make it easier to write accurate commit messages; like, in the extreme where it's just one big commit, it might say "compose: Implement redesigned compose box". @gnprice may have thoughts on this.
A sequence of small, incremental commits is definitely the right choice in some situations. Here, though, I think it's less important to chart out the path from "here" to "there" for a reviewer to follow. The main focus in review will just be to find and understand any differences from the Figma.
| // Leaving `contentPadding` out would still result in the same layout. | ||
| // For better predictability, set it explicitly instead of relying on | ||
| // the default value. | ||
| contentPadding: EdgeInsets.zero, |
There was a problem hiding this comment.
This didn't turn out to be true in my testing just now:
with EdgeInsets.zero |
left out |
|---|---|
I see the layout shifting if I open these images in separate browser tabs and switch between them.
Fixes: #915