Adjust MarkAsReadWidget text to be bolder, use Source Sans 3 #376

sirpengi · 2023-11-11T15:24:05Z

Thanks @chrisbobbe for the report: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1679769

Adjusted border radius to 7
Restyled text per recommendations, specifying line height, and adjusting button height to be explicit (used 48px in this case, to match button target recommendations)

New screenshot:

chrisbobbe

Thanks @sirpengi! One small comment below.

chrisbobbe · 2023-11-11T19:46:22Z

lib/widgets/message_list.dart

+                textStyle: const TextStyle(
+                  fontFamily: 'Source Sans 3',
+                  fontSize: 18,
+                  fontWeight: FontWeight.w400,


The fontWeight: FontWeight.w400, is redundant with .merge(weightVariableTextStyle(context)).

sirpengi · 2023-11-12T11:16:40Z

@chrisbobbe readjusted per your recommendations!

sirpengi · 2023-11-12T23:27:53Z

Readjusted button height to 38px based on https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680573

gnprice

Thanks for fixing these! Comments below.

gnprice · 2023-11-13T22:37:32Z

lib/widgets/message_list.dart

-                padding: const EdgeInsets.all(10),
-                textStyle: const TextStyle(fontSize: 18, fontWeight: FontWeight.w200),
-                shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(5)),
+                backgroundColor: const HSLColor.fromAHSL(1,227,0.78,0.59).toColor(),


nit: space after comma

Also Chris observed in that chat thread that this is the same color as the unread markers on messages, and that seems like not a coincidence — those markers are what this button will eliminate, and the shared color draws the connection. So let's factor it out with a name. Can be a static on _UnreadMarker, like static final color = ….

gnprice · 2023-11-13T22:55:47Z

lib/widgets/message_list.dart

-                textStyle: const TextStyle(fontSize: 18, fontWeight: FontWeight.w200),
-                shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(5)),
+                backgroundColor: const HSLColor.fromAHSL(1,227,0.78,0.59).toColor(),
+                fixedSize: const Size.fromHeight(38),


This seems to have no effect.

Looking with the inspector:

The button's height as a tap target is 48px. That's because Flutter's Material implementation is providing that behavior for us automatically, which is handy.

Its height as it participates in layout is also 48px. That's fine but probably means we want to reduce the further padding we give it above and below.

The height of the visible button area is 40px. That reflects the default for ButtonStyle.minimumSize for FilledButton.

So we can override minimumSize here to get the visible height to 38px. The 48px for the touchable height is good and we can leave that as it is. But then the surrounding padding should be adjusted to reflect that the FilledButton already contains some padding for the touch target.

From what I can tell setting fixedSize would have the same visual effect as setting minimumSize, the logic in _ButtonStyleState.build shows it as being the final decision on the constraints of the button size (as does poking around the inspector and tweaking the values there). On reflection, it would be safer to set a minimum instead in case the button contents expand beyond the button, allowing it to grow vertically. In any case, switched to mininumSize and reduced the containing vertical padding by a total of 10px.

Well, I started from just an empirical observation: this line with fixedSize had no effect. If I commented it out, nothing changed. Then the further empirical observation is that the inspector shows the height of the visible button area as 40px, which is more than 38px.

OTOH when I tried setting minimumSize here to a small value, that did have an effect — the button got shorter. So minimumSize does have an effect where fixedSize doesn't, even if the code that implements that is a bit tangly.

Looking at the linked code, we have:

final Size? resolvedMinimumSize = resolve<Size?>((ButtonStyle? style) => style?.minimumSize); final Size? resolvedFixedSize = resolve<Size?>((ButtonStyle? style) => style?.fixedSize); // … BoxConstraints effectiveConstraints = resolvedVisualDensity.effectiveConstraints( BoxConstraints( minWidth: resolvedMinimumSize!.width, minHeight: resolvedMinimumSize.height, maxWidth: resolvedMaximumSize!.width, maxHeight: resolvedMaximumSize.height, ), ); if (resolvedFixedSize != null) { final Size size = effectiveConstraints.constrain(resolvedFixedSize); // … width … if (size.height.isFinite) { effectiveConstraints = effectiveConstraints.copyWith( minHeight: size.height, maxHeight: size.height, ); } }

So minimumSize — after resolving a value for it using the defaults, which here means 40px height — feeds into effectiveConstraints, and then that meets fixedSize in this line:

final Size size = effectiveConstraints.constrain(resolvedFixedSize);

When they disagree, then, which one wins? The answer is up to that BoxConstraints.constrain method. Its docs say:

Returns the size that both satisfies the constraints and is as close as possible to the given size.

So the resulting size will satisfy the constraints; it may or may not agree with the size, and will only be as close to the given size as the constraints allow. The constraints win any disagreement.

Here, that means effectiveConstraints, and through it minimumSize, prevail over fixedSize.

I see now, and had a closer inspection of the code and BoxContraints.constrain and stand (massively) corrected. I must have only tested with sizes > 40 as well? I did see a change as I adjusted the value but perhaps I didn't test with smaller values.

sirpengi · 2023-11-14T14:07:43Z

@gnprice good to go again!

gnprice · 2023-11-15T02:41:19Z

lib/widgets/message_list.dart

+  //   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
+  static Color color = const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor();


Suggested change

static Color color = const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor();

static final color = const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor();

That way it's clear this is meant to function as a constant, and not to ever get mutated.

gnprice · 2023-11-15T02:43:27Z

lib/widgets/message_list.dart

-                padding: const EdgeInsets.all(10),
-                textStyle: const TextStyle(fontSize: 18, fontWeight: FontWeight.w200),
-                shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(5)),
+                backgroundColor: _UnreadMarker.color,


msglist [nfc]: Factor out color for _UnreadMarker and MarkAsReadWidget

This change isn't NFC, because it changes the color of MarkAsReadWidget.

Could have an NFC commit without this line, just pulling out the static _UnreadMarker.color, and then a second commit to use it here. Or this commit is fine as is, it just isn't NFC (and what it's doing isn't "factoring out" from MarkAsReadWidget, because this color wasn't previously there) — a good commit message would be msglist: Color MarkAsReadWidget the same as _UnreadMarker.

gnprice · 2023-11-15T02:55:38Z

lib/widgets/message_list.dart

-                textStyle: const TextStyle(fontSize: 18, fontWeight: FontWeight.w200),
-                shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(5)),
+                backgroundColor: const HSLColor.fromAHSL(1,227,0.78,0.59).toColor(),
+                fixedSize: const Size.fromHeight(38),


Well, I started from just an empirical observation: this line with fixedSize had no effect. If I commented it out, nothing changed. Then the further empirical observation is that the inspector shows the height of the visible button area as 40px, which is more than 38px.

OTOH when I tried setting minimumSize here to a small value, that did have an effect — the button got shorter. So minimumSize does have an effect where fixedSize doesn't, even if the code that implements that is a bit tangly.

Looking at the linked code, we have:

final Size? resolvedMinimumSize = resolve<Size?>((ButtonStyle? style) => style?.minimumSize); final Size? resolvedFixedSize = resolve<Size?>((ButtonStyle? style) => style?.fixedSize); // … BoxConstraints effectiveConstraints = resolvedVisualDensity.effectiveConstraints( BoxConstraints( minWidth: resolvedMinimumSize!.width, minHeight: resolvedMinimumSize.height, maxWidth: resolvedMaximumSize!.width, maxHeight: resolvedMaximumSize.height, ), ); if (resolvedFixedSize != null) { final Size size = effectiveConstraints.constrain(resolvedFixedSize); // … width … if (size.height.isFinite) { effectiveConstraints = effectiveConstraints.copyWith( minHeight: size.height, maxHeight: size.height, ); } }

So minimumSize — after resolving a value for it using the defaults, which here means 40px height — feeds into effectiveConstraints, and then that meets fixedSize in this line:

final Size size = effectiveConstraints.constrain(resolvedFixedSize);

When they disagree, then, which one wins? The answer is up to that BoxConstraints.constrain method. Its docs say:

Returns the size that both satisfies the constraints and is as close as possible to the given size.

So the resulting size will satisfy the constraints; it may or may not agree with the size, and will only be as close to the given size as the constraints allow. The constraints win any disagreement.

Here, that means effectiveConstraints, and through it minimumSize, prevail over fixedSize.

gnprice

Thanks @sirpengi! Just a few nits, below and above. Also a reply above about the interaction between minimumSize and fixedSize on these Material button widgets, though that doesn't come with any further action items since you've already switched to minimumSize.

gnprice · 2023-11-15T03:00:19Z

lib/widgets/message_list.dart

@@ -392,12 +393,18 @@ class MarkAsReadWidget extends StatelessWidget {
          // TODO(#368): this should pull from stream color
          color: Colors.transparent,
          child: Padding(
-            padding: const EdgeInsets.all(10),
+            // vertical padding adjusted for tap target height (48px) of containing button


nit:

Suggested change

// vertical padding adjusted for tap target height (48px) of containing button

// vertical padding adjusted for tap target height (48px) of button

From the perspective of this Padding, the button isn't containing, but rather contained — it's the child of the padding.

gnprice · 2023-11-15T20:14:57Z

(I see you pushed another revision — is this meant to be ready for re-review?)

sirpengi · 2023-11-16T11:29:10Z

@gnprice yes, this is ready for review! (I usually wait for checks to pass and then forgot about it!)

Use Source Sans 3 for font family and bolder weight. Readjust containing padding due to tap target size of button being larger than visible height of button.

gnprice · 2023-11-16T19:26:15Z

Cool. Thanks for the revision — looks good, merging.

sirpengi requested review from gnprice and chrisbobbe November 11, 2023 15:24

chrisbobbe reviewed Nov 11, 2023

View reviewed changes

sirpengi force-pushed the pr-mark-read-widget-style branch 3 times, most recently from a9d4111 to 531cae1 Compare November 12, 2023 11:12

sirpengi force-pushed the pr-mark-read-widget-style branch from 531cae1 to 2329bc3 Compare November 12, 2023 11:35

sirpengi force-pushed the pr-mark-read-widget-style branch from 2329bc3 to 0cd3809 Compare November 12, 2023 23:28

gnprice reviewed Nov 13, 2023

View reviewed changes

sirpengi force-pushed the pr-mark-read-widget-style branch from 0cd3809 to efe3708 Compare November 14, 2023 13:48

gnprice reviewed Nov 15, 2023

View reviewed changes

sirpengi force-pushed the pr-mark-read-widget-style branch from efe3708 to 4974433 Compare November 15, 2023 10:19

sirpengi added 2 commits November 16, 2023 11:24

msglist: Color MarkAsReadWidget the same as _UnreadMarker

34bc872

msglist: Adjust MarkAsReadWidget style for text, size, and border radius

45f223c

Use Source Sans 3 for font family and bolder weight. Readjust containing padding due to tap target size of button being larger than visible height of button.

gnprice force-pushed the pr-mark-read-widget-style branch from 4974433 to 45f223c Compare November 16, 2023 19:26

gnprice merged commit 45f223c into zulip:main Nov 16, 2023

sirpengi deleted the pr-mark-read-widget-style branch January 23, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust MarkAsReadWidget text to be bolder, use Source Sans 3 #376

Adjust MarkAsReadWidget text to be bolder, use Source Sans 3 #376

sirpengi commented Nov 11, 2023 •

edited

Loading

chrisbobbe left a comment

chrisbobbe Nov 11, 2023

sirpengi commented Nov 12, 2023

sirpengi commented Nov 12, 2023

gnprice left a comment

gnprice Nov 13, 2023

gnprice Nov 13, 2023

sirpengi Nov 14, 2023 •

edited

Loading

gnprice Nov 15, 2023

sirpengi Nov 15, 2023

sirpengi commented Nov 14, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice left a comment

gnprice Nov 15, 2023

gnprice commented Nov 15, 2023

sirpengi commented Nov 16, 2023

gnprice commented Nov 16, 2023

	static Color color = const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor();
	static final color = const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor();

	// vertical padding adjusted for tap target height (48px) of containing button
	// vertical padding adjusted for tap target height (48px) of button

Adjust MarkAsReadWidget text to be bolder, use Source Sans 3 #376

Adjust MarkAsReadWidget text to be bolder, use Source Sans 3 #376

Conversation

sirpengi commented Nov 11, 2023 • edited Loading

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Nov 12, 2023

sirpengi commented Nov 12, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Nov 15, 2023

sirpengi commented Nov 16, 2023

gnprice commented Nov 16, 2023

sirpengi commented Nov 11, 2023 •

edited

Loading

sirpengi Nov 14, 2023 •

edited

Loading