-
Notifications
You must be signed in to change notification settings - Fork 134
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
Compose box redesign #928
base: main
Are you sure you want to change the base?
Compose box redesign #928
Conversation
c17497f
to
a566c67
Compare
(getting the screenshots ready) |
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 |
9f9fb43
to
340a5ce
Compare
I think I have found a true simple solution to this: set |
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! |
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]>
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.
Exciting! It feels nice to be moving forward with our new, finalized designs. 🙂
Comments below from a light review; I may have more, later, when I have time to do a deeper review.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes: #915