-
Notifications
You must be signed in to change notification settings - Fork 245
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: Compose-box border is subtle in dark mode #1233
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks! I see this is a draft, but I gave it a quick skim and I noticed a different direction I'd like to take. 🙂
Since the shadow is only meant to be applied in dark theme, I'd like to avoid asking Flutter to do any computations for it when in light mode. For that, let's add a ComposeBoxTheme
class in lib/widgets/compose_box.dart, with just one field boxShadow
that's null in light theme and non-null in dark theme. ContentTheme
and MessageListTheme
are examples of the pattern I have in mind. So, something like this:
Proposal
/// Compose-box styles that differ between light and dark theme.
///
/// These styles will animate on theme changes (with help from [lerp]).
class ComposeBoxTheme extends ThemeExtension<ComposeBoxTheme> {
factory ComposeBoxTheme.light(BuildContext context) {
return ComposeBoxTheme._(
boxShadow: null,
);
}
factory ComposeBoxTheme.dark(BuildContext context) {
return ComposeBoxTheme._(
boxShadow: [BoxShadow(
color: DesignVariables.dark.bgTopBar,
offset: const Offset(0, -4),
blurRadius: 16,
spreadRadius: 0,
)],
);
}
ComposeBoxTheme._({
required this.boxShadow,
});
/// The [ComposeBoxTheme] from the context's active theme.
///
/// The [ThemeData] must include [ComposeBoxTheme] in [ThemeData.extensions].
static ComposeBoxTheme of(BuildContext context) {
final theme = Theme.of(context);
final extension = theme.extension<ComposeBoxTheme>();
assert(extension != null);
return extension!;
}
final List<BoxShadow>? boxShadow;
@override
ComposeBoxTheme copyWith({
List<BoxShadow>? boxShadow,
}) {
return ComposeBoxTheme._(
boxShadow: boxShadow ?? this.boxShadow,
);
}
@override
ComposeBoxTheme lerp(ComposeBoxTheme other, double t) {
if (identical(this, other)) {
return this;
}
return ComposeBoxTheme._(
boxShadow: BoxShadow.lerpList(boxShadow, other.boxShadow, t)!,
);
}
}
Then the caller can say boxShadow: ComposeBoxTheme.of(context).boxShadow
.
Note the use of DesignVariables.dark.bgTopBar
instead of adding a new design variable. The Figma doesn't add any new design variables. Also, it's DesignVariables.dark.bgTopBar
instead of DesignVariables.dark().bgTopBar
because of a small optimization, #1236; I suggest rebasing your work atop that PR to get that optimization.
Thanks for the review, I am working on it and will update it as required ASAP. |
8f80b77
to
6f68abc
Compare
There is a merge conflict. Could you resolve it? Thanks! |
c6bb562
to
260457a
Compare
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.
🥳
260457a
to
0764a86
Compare
Updated the top border and added shadow effect to the top of the compose box to ensure it is subtle in dark mode. For reference, see the Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5908-64038&t=alSmAmdRXFDwT4IT-1 Fixes: zulip#1207
0764a86
to
504f5f2
Compare
Hello, @chrisbobbe thanks for mentioning the changes. I have updated it accordingly. |
Updated the top border and added shadow effect to the top of the compose box to ensure it is subtle in dark mode.
For reference, see the Figma design:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5908-64038&t=alSmAmdRXFDwT4IT-1
Fixes: #1207