Skip to content
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.

Sign up for GitHub

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

Jump to bottom

msglist: Update more elements closer to new, 2023+ design, to prepare for light/dark #686

Merged
merged 18 commits into from
May 20, 2024
Merged

msglist: Update more elements closer to new, 2023+ design, to prepare for light/dark #686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8f25a7c
msglist [nfc]: s/backgroundColor/appBarBackgroundColor/ for explicitness
chrisbobbe May 16, 2024
42c5a68
msglist [nfc]: Comment on unread-marker color for dark theme
chrisbobbe May 16, 2024
ee57be0
msglist [nfc]: Cut unnecessary condition on bar-background brightness
chrisbobbe May 17, 2024
6abafdd
msglist [nfc]: Inline a variable
chrisbobbe May 17, 2024
7100ac5
msglist: Follow web and Figma in recipient-header text color
chrisbobbe May 17, 2024
822e6ae
msglist [nfc]: Deduplicate recipient-header text style
chrisbobbe May 17, 2024
662a710
msglist: In unsubscribed stream, make icon color match text color
chrisbobbe May 17, 2024
20c08c8
msglist [nfc]: Inline a variable
chrisbobbe May 17, 2024
4c88148
msglist [nfc]: Remove RecipientHeaderDate unnecessary `color` param
chrisbobbe May 17, 2024
9275940
msglist: Follow web in recipient-header-date color
chrisbobbe May 17, 2024
1fd4676
msglist: Make date-separator text color match recipient-header dates
chrisbobbe May 17, 2024
41acd77
msglist [nfc]: Remove DateText unnecessary `color` param
chrisbobbe May 17, 2024
2c18dc5
msglist: Follow web in chevron-right icon color in stream recipient h…
chrisbobbe May 17, 2024
c0a75d0
msglist [nfc]: Update a stale comment about zulip/zulip code
chrisbobbe May 17, 2024
72f0fd7
msglist [nfc]: Add some TODO(#95)s
chrisbobbe May 20, 2024
a027921
msglist test: Move two stream-recipient-header tests into their group
chrisbobbe May 19, 2024
42db773
msglist test: Make new DmRecpientHeader group; move two tests into it
chrisbobbe May 19, 2024
1439b8d
msglist: Make DM recipient header icon (ZulipIcons.user) match text c…
chrisbobbe May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 46 additions & 52 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,35 +58,35 @@ class _MessageListPageState extends State<MessageListPage> {
Widget build(BuildContext context) {
final store = PerAccountStoreWidget.of(context);

final Color? backgroundColor;
final Color? appBarBackgroundColor;
bool removeAppBarBottomBorder = false;
switch(widget.narrow) {
case AllMessagesNarrow():
backgroundColor = null; // i.e., inherit
appBarBackgroundColor = null; // i.e., inherit

case StreamNarrow(:final streamId):
case TopicNarrow(:final streamId):
backgroundColor = store.subscriptions[streamId]?.colorSwatch().barBackground
appBarBackgroundColor = store.subscriptions[streamId]?.colorSwatch().barBackground
?? _kUnsubscribedStreamRecipientHeaderColor;
// All recipient headers will match this color; remove distracting line
// (but are recipient headers even needed for topic narrows?)
removeAppBarBottomBorder = true;

case DmNarrow():
backgroundColor = _kDmRecipientHeaderColor;
appBarBackgroundColor = _kDmRecipientHeaderColor;
// All recipient headers will match this color; remove distracting line
// (but are recipient headers even needed?)
removeAppBarBottomBorder = true;
}

return Scaffold(
appBar: AppBar(title: MessageListAppBarTitle(narrow: widget.narrow),
backgroundColor: backgroundColor,
backgroundColor: appBarBackgroundColor,
shape: removeAppBarBottomBorder
? const Border()
: null, // i.e., inherit
),
// TODO question for Vlad: for a stream view, should we set
// TODO question for Vlad: for a stream view, should we set the Scaffold's
// [backgroundColor] based on stream color, as in this frame:
// https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132%3A9684&mode=dev
// That's not obviously preferred over the default background that
Expand Down Expand Up @@ -475,6 +475,7 @@ class MarkAsReadWidget extends StatelessWidget {
padding: const EdgeInsets.symmetric(horizontal: 10, vertical: 10 - ((48 - 38) / 2)),
child: FilledButton.icon(
style: FilledButton.styleFrom(
// TODO(#95) need dark-theme colors (foreground and background)
backgroundColor: _UnreadMarker.color,
minimumSize: const Size.fromHeight(38),
textStyle:
Expand Down Expand Up @@ -519,8 +520,8 @@ class DateSeparator extends StatelessWidget {

final Message message;

// This color matches recipient headers. TODO(design) is that what we want?
static final _textColor = Colors.black.withOpacity(0.4);
// TODO(#95) in dark theme, use white, following web
static const _line = BorderSide(width: 0, color: Colors.black);

@override
Widget build(BuildContext context) {
Expand All @@ -537,22 +538,17 @@ class DateSeparator extends StatelessWidget {
child: DecoratedBox(
decoration: BoxDecoration(
border: Border(
bottom: BorderSide(
width: 0,
color: Colors.black)))))),
bottom: _line))))),
Padding(padding: const EdgeInsets.fromLTRB(2, 0, 2, textBottomPadding),
child: DateText(
color: _textColor,
fontSize: 16,
height: (16 / 16),
timestamp: message.timestamp)),
const SizedBox(height: 0, width: 12,
child: DecoratedBox(
decoration: BoxDecoration(
border: Border(
bottom: BorderSide(
width: 0,
color: Colors.black)))))
bottom: _line)))),
])),
);
}
Expand Down Expand Up @@ -598,6 +594,13 @@ class _UnreadMarker extends StatelessWidget {
// https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132-9684
// See discussion about design at:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20unread.20marker/near/1658008
// TODO(#95) use opacity 0.75 in dark theme? Discussion, a few weeks after the
// above-linked discussion:
// https://github.com/zulip/zulip-flutter/pull/317#issuecomment-1784311663
// where Vlad includes screenshots that look like they're from the Figma here:
// https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=807-33998&t=81Z9nyeJmsdmLgq4-0
// (Web uses a left-to-right gradient from hsl(217deg 64% 59%) to transparent,
// in both light and dark theme.)
static final color = const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor();

@override
Expand Down Expand Up @@ -649,27 +652,15 @@ class StreamMessageRecipientHeader extends StatelessWidget {

final subscription = store.subscriptions[message.streamId];
final Color backgroundColor;
final Color contrastingColor;
Copy link
Member

Choose a reason for hiding this comment

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

Let's squash these two commits together:
71b4135 msglist [nfc]: Simplify a variable into a const
91ceae8 msglist [nfc]: Inline a variable

The squashed diff is still small and easy to follow, and I think makes the direction the refactor is going clearer than the first commit does alone.

final Color iconColor;
if (subscription != null) {
final swatch = subscription.colorSwatch();
backgroundColor = swatch.barBackground;
contrastingColor =
(ThemeData.estimateBrightnessForColor(swatch.barBackground) == Brightness.dark)
? Colors.white
: Colors.black;
iconColor = swatch.iconOnBarBackground;
} else {
backgroundColor = _kUnsubscribedStreamRecipientHeaderColor;
contrastingColor = Colors.black;
iconColor = Colors.black;
iconColor = _kRecipientHeaderTextColor;
}
final textStyle = TextStyle(
color: contrastingColor,
fontSize: 16,
letterSpacing: proportionalLetterSpacing(context, 0.02, baseFontSize: 16),
height: (18 / 16),
).merge(weightVariableTextStyle(context, wght: 600));

final Widget streamWidget;
if (!showStream) {
Expand Down Expand Up @@ -698,15 +689,15 @@ class StreamMessageRecipientHeader extends StatelessWidget {
Padding(
padding: const EdgeInsets.symmetric(vertical: 11),
child: Text(streamName,
style: textStyle,
style: recipientHeaderTextStyle(context),
overflow: TextOverflow.ellipsis),
),
Padding(
// Figma has 5px horizontal padding around an 8px wide icon.
// Icon is 16px wide here so horizontal padding is 1px.
padding: const EdgeInsets.symmetric(horizontal: 1),
child: Icon(size: 16,
color: contrastingColor.withOpacity(0.6),
color: Colors.black.withOpacity(0.3),
ZulipIcons.chevron_right)),
]));
}
Expand All @@ -729,11 +720,10 @@ class StreamMessageRecipientHeader extends StatelessWidget {
// TODO: Give a way to see the whole topic (maybe a
// long-press interaction?)
overflow: TextOverflow.ellipsis,
style: textStyle))),
style: recipientHeaderTextStyle(context)))),
// TODO topic links?
// Then web also has edit/resolve/mute buttons. Skip those for mobile.
RecipientHeaderDate(message: message,
color: contrastingColor.withOpacity(0.4)),
RecipientHeaderDate(message: message),
])));
}
}
Expand Down Expand Up @@ -770,41 +760,45 @@ class DmRecipientHeader extends StatelessWidget {
child: Row(
crossAxisAlignment: CrossAxisAlignment.center,
children: [
const Padding(
padding: EdgeInsets.symmetric(horizontal: 6),
child: Icon(size: 16, ZulipIcons.user)),
Padding(
padding: const EdgeInsets.symmetric(horizontal: 6),
child: Icon(
color: _kRecipientHeaderTextColor,
size: 16,
ZulipIcons.user)),
Expanded(
child: Text(title,
style: TextStyle(
fontSize: 16,
letterSpacing: proportionalLetterSpacing(context, 0.02, baseFontSize: 16),
height: (18 / 16),
).merge(weightVariableTextStyle(context, wght: 600)),
style: recipientHeaderTextStyle(context),
overflow: TextOverflow.ellipsis)),
RecipientHeaderDate(message: message,
color: _kDmRecipientHeaderDateColor),
RecipientHeaderDate(message: message),
]))));
}
}

// TODO(#95): web uses different color in dark mode
// Header color from `color-background-private-message-header`
// in zulip:web/styles/zulip.css .
// --color-background-private-message-header in web/styles/app_variables.css
final _kDmRecipientHeaderColor = const HSLColor.fromAHSL(1, 46, 0.35, 0.93).toColor();
final _kDmRecipientHeaderDateColor = Colors.black.withOpacity(0.4);

TextStyle recipientHeaderTextStyle(BuildContext context) {
return TextStyle(
color: _kRecipientHeaderTextColor,
fontSize: 16,
letterSpacing: proportionalLetterSpacing(context, 0.02, baseFontSize: 16),
height: (18 / 16),
).merge(weightVariableTextStyle(context, wght: 600));
}
final _kRecipientHeaderTextColor = const HSLColor.fromAHSL(1, 0, 0, 0.15).toColor();

class RecipientHeaderDate extends StatelessWidget {
const RecipientHeaderDate({super.key, required this.message, required this.color});
const RecipientHeaderDate({super.key, required this.message});

final Message message;
final Color color;

@override
Widget build(BuildContext context) {
return Padding(
padding: const EdgeInsets.fromLTRB(10, 0, 16, 0),
child: DateText(
color: color,
fontSize: 16,
// In Figma this has a line-height of 19, but using 18
// here to match the stream/topic text widgets helps
Expand All @@ -817,13 +811,11 @@ class RecipientHeaderDate extends StatelessWidget {
class DateText extends StatelessWidget {
const DateText({
super.key,
required this.color,
required this.fontSize,
required this.height,
required this.timestamp,
});

final Color color;
final double fontSize;
final double height;
final int timestamp;
Expand All @@ -833,7 +825,7 @@ class DateText extends StatelessWidget {
final zulipLocalizations = ZulipLocalizations.of(context);
return Text(
style: TextStyle(
color: color,
color: const HSLColor.fromAHSL(0.75, 0, 0, 0.15).toColor(),
Comment on lines -836 to +828
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if some confusion about contrastingColor (see a recent
commit where we removed a variable with that name) might have led to
the old color being used for stream headers, and perhaps the
DM-header change was just done to align with stream headers.

Yeah. Glad to have this cleaned up! And good to have it consolidated here on DateText, too.

fontSize: fontSize,
height: height,
// This is equivalent to css `all-small-caps`, see:
Expand Down Expand Up @@ -892,6 +884,7 @@ class MessageWithPossibleSender extends StatelessWidget {

final MessageListMessageItem item;

// TODO(#95) unchanged in dark theme?
Copy link
Member

Choose a reason for hiding this comment

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

Let's squash these three:

14c3f52 msglist [nfc]: Comment on _starColor about dark theme
a0ebda6 msglist [nfc]: Comment on _kMessageTimestampColor about dark theme
61be55b msglist [nfc]: Comment with TODO(#95) on mark-as-read button

oh and this previous commit:
53440c7 msglist [nfc]: Add TODO on date-separator line about dark theme

They're one line each, all adding comments on the same theme, and none of them call for significant discussion in the commit message.

static final _starColor = const HSLColor.fromAHSL(0.5, 47, 1, 0.41).toColor();

@override
Expand Down Expand Up @@ -984,6 +977,7 @@ class MessageWithPossibleSender extends StatelessWidget {
// TODO web seems to ignore locale in formatting time, but we could do better
final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US');

// TODO(#95) need dark-theme color (this one comes from the Figma)
final _kMessageTimestampColor = const HSLColor.fromAHSL(1, 0, 0, 0.5).toColor();

Future<void> markNarrowAsRead(
Expand Down
1 change: 1 addition & 0 deletions test/flutter_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ extension TextFieldChecks on Subject<TextField> {

extension TextStyleChecks on Subject<TextStyle> {
Subject<bool> get inherit => has((t) => t.inherit, 'inherit');
Subject<Color?> get color => has((t) => t.color, 'color');
Subject<double?> get fontSize => has((t) => t.fontSize, 'fontSize');
Subject<FontWeight?> get fontWeight => has((t) => t.fontWeight, 'fontWeight');
Subject<double?> get letterSpacing => has((t) => t.letterSpacing, 'letterSpacing');
Expand Down
Loading
Loading