Edit-message (3/n): Implement ZulipWebUiKitButton, for "Cancel" / "Save" buttons #1432

chrisbobbe · 2025-03-21T22:58:01Z

This PR implements some variants of the "Button" component in the "Zulip Web UI kit" in Figma: https://www.figma.com/design/msWyAJ8cnMHgOMPxi7BUvA/Zulip-Web-UI-kit?node-id=1-8&p=f&m=dev

The Figma uses this component for the "Cancel" and "Save" buttons in the edit-message compose box:

https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3988-38200&m=dev

Light	Dark

Not all values of the "intent" and "attention" params are implemented here; just the "info" intent with "medium" and "high" attention. In the edit-message compose box banner, both buttons have the "info" intent; the cancel button has "medium" attention and the save button has "high" attention.

Here are screenshots of the buttons, made with these commits and also #1430 (plus the _EditMessageBanner class in that PR's description):

Cancel button ("info" intent, "medium" attention)

Light; normal state	Light; pressed state	Dark; normal state	Dark; pressed state

Save button ("info" intent, "high" attention)

Light; normal state	Light; pressed state	Dark; normal state	Dark; pressed state

Related: #126

gnprice · 2025-03-21T23:13:16Z

lib/widgets/button.dart

+enum ZulipWebUiKitButtonIntent {
+  // neutral,
+  // warning,
+  // danger,
+  info,
+  // success,
+  // brand,


One good reason to add these only lazily, as you're doing here: to keep down how many different colors we have to maintain in theme.dart, until we finish #831.

PIG208

This is neat. Thanks for working on this! Left some comments.

PIG208 · 2025-03-25T01:01:10Z

lib/widgets/theme.dart

+    required this.btnLabelAttMediumIntInfo,
    required this.btnLabelAttMediumIntDanger,


nit: order

Suggested change

required this.btnLabelAttMediumIntInfo,

required this.btnLabelAttMediumIntDanger,

required this.btnLabelAttMediumIntDanger,

required this.btnLabelAttMediumIntInfo,

PIG208 · 2025-03-25T01:21:02Z

lib/widgets/button.dart

+        ).copyWith(backgroundColor: _backgroundColor(designVariables)),
+        onPressed: onPressed,
+        child: ConstrainedBox(
+          constraints: BoxConstraints(maxWidth: 240),


I wonder if we need a different value for the mobile app. Presumably this is for web.

If 240 doesn't seem OK, could you say more about why? If it's OK but just not ideal, I think it's fine to leave it as-is and tune later based on feedback or more design work.

Usually when we have wider-than-usual buttons, they occupy the screen-width or the width of the parent; for my phone a 240px button would be a bit narrower than my screen. I think leaving it this way and tuning later sounds like a good plan.

PIG208 · 2025-03-25T01:22:46Z

lib/widgets/button.dart

+
+    return AnimatedScaleOnTap(
+      scaleEnd: 0.96,
+      duration: Duration(milliseconds: 50),


Looks like duration in Figma is 100ms:

Thanks for spotting that! I didn't see it, so instead found an animation in the web app with Chrome dev tools; probably ought to have made a comment about that :)

PIG208 · 2025-03-25T01:28:27Z

lib/widgets/button.dart

+  final ZulipWebUiKitButtonAttention attention;
+  final ZulipWebUiKitButtonIntent intent;
+  final String label;
+  final VoidCallback? onPressed;


Looks like the Figma design does not offer style for disabled buttons. Widgets like TextButton are disabled when onPressed is null, but we don't have special styling to indicate that (and AnimatedScaleOnTap still works).

PIG208 · 2025-03-25T01:47:58Z

lib/widgets/theme.dart

@@ -188,8 +195,15 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
    bgTopBar: const Color(0xff242424),
    borderBar: const Color(0xffffffff).withValues(alpha: 0.1),
    borderMenuButtonSelected: Colors.white.withValues(alpha: 0.1),
+    btnBgAttHighIntInfoActive: const Color(0xff1e41d3),
+    btnBgAttHighIntInfoNormal: const Color(0xff1e41d3),
+    btnBgAttMediumIntInfoActive: const Color(0xff97b6fe).withValues(alpha: 0.22),


This should be 0.12 as well.

Thanks for the catch!

chrisbobbe · 2025-04-01T04:27:52Z

Thanks for the review! Revision pushed.

PIG208 · 2025-04-01T14:54:39Z

Thanks for the update! This LGTM.

gnprice

Thanks! This structure looks good; a couple of comments below.

gnprice · 2025-04-02T01:34:31Z

lib/widgets/button.dart

+    return TextStyle(
+      color: _labelColor(designVariables),
+      fontSize: 17,
+      height: 1.20,
+      letterSpacing: proportionalLetterSpacing(context,
+        0.006, baseFontSize: 17),
+    ).merge(weightVariableTextStyle(context, wght: 600));


Where do these details come from?

Looking at one of the buttons in Figma:
https://www.figma.com/design/msWyAJ8cnMHgOMPxi7BUvA/Zulip-Web-UI-kit?node-id=1-2960&m=dev
I see:

overflow: hidden; color: var(--btn-label-att_high, #FFF); text-align: center; text-overflow: ellipsis; font-family: "Source Sans 3"; font-size: 16px; font-style: normal; font-weight: 500; line-height: 20px; /* 125% */ letter-spacing: 0.096px;

Odd, I took these details from the buttons as they appear in the compose-box frame:

https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3988-38201&m=dev

overflow: hidden; color: var(--btn-label-att_high, #FFF); text-align: center; text-overflow: ellipsis; font-family: "Source Sans 3"; font-size: 17px; font-style: normal; font-weight: 600; line-height: 120%; /* 20.4px */ letter-spacing: 0.102px;

Huh. Yeah, I guess leave a comment, then. We can leave this code specialized to what that bit of the mobile design calls for, unless we find that other designs call for something different in which case we can generalize as needed.

gnprice · 2025-04-02T02:09:24Z

lib/widgets/button.dart

+            borderRadius: BorderRadius.circular(4)),
+          splashFactory: NoSplash.splashFactory,
+          tapTargetSize: MaterialTapTargetSize.shrinkWrap,
+          minimumSize: Size(kMinInteractiveDimension, 28),


Hmm, 28px is awfully small for a touch target.

The Figma design says:

People might say that 44px is recommended minimum for touch screens but we assume that this is for web app.

Here we are indeed talking about a touch screen, though 🙂 So being just 28px high, rather than 44px or 48px, seems likely to be frustrating.

It'd be fine if the visual button is on the short side, as long as the tap target is a good height. So in the edit-message design, it calls for the buttons to be 28px high but then the whole banner is 40px. So making the banner only slightly taller than the design would make room for a 44px tap target, which could still enclose a 28px visual button.

These Material buttons have a facility for padding out the tap target beyond the visual button — see the call site of _InputPadding in button_style_button.dart (in _ButtonStyleState.build). Can we use that to get that effect?

…dart

chrisbobbe · 2025-04-04T05:38:30Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! The implementation all looks good. Small comments from reading through the tests.

gnprice · 2025-04-04T18:08:29Z

test/widgets/button_test.dart

+    final textScaleFactorVariants = ValueVariant(Set.of(kTextScaleFactors));
+    testWidgets('button and touch-target heights', (tester) async {
+      addTearDown(testBinding.reset);
+      tester.platformDispatcher.textScaleFactorTestValue = textScaleFactorVariants.currentValue!;


Huh, neat mechanism.

gnprice · 2025-04-04T18:08:39Z

test/widgets/button_test.dart

+    final textScaleFactorVariants = ValueVariant(Set.of(kTextScaleFactors));
+    testWidgets('button and touch-target heights', (tester) async {


nit:

Suggested change

final textScaleFactorVariants = ValueVariant(Set.of(kTextScaleFactors));

testWidgets('button and touch-target heights', (tester) async {

final textScaleFactorVariants = ValueVariant(Set.of(kTextScaleFactors));

testWidgets('button and touch-target heights', (tester) async {

gnprice · 2025-04-04T18:10:30Z

test/widgets/button_test.dart

+      final renderObject = element.renderObject as RenderBox;
+      final width = renderObject.size.width;
+      final buttonTopLeft = tester.getTopLeft(buttonFinder);
+      final buttonCenter = tester.getCenter(buttonFinder);
+      check(element).size.isNotNull().height.equals(44); // includes outer padding


Is there a difference between element.size and (element.renderObject as RenderBox).size? I notice this code is using one of those to find the height and the other for the width.

gnprice · 2025-04-04T18:12:20Z

test/widgets/button_test.dart

+      final buttonTopLeft = tester.getTopLeft(buttonFinder);
+      final buttonCenter = tester.getCenter(buttonFinder);
+      check(element).size.isNotNull().height.equals(44); // includes outer padding
+
+      // Outer padding responds to taps, not just the painted part.


nit: these two are used only here in this next stanza, so move them there

Suggested change

final buttonTopLeft = tester.getTopLeft(buttonFinder);

final buttonCenter = tester.getCenter(buttonFinder);

check(element).size.isNotNull().height.equals(44); // includes outer padding

// Outer padding responds to taps, not just the painted part.

check(element).size.isNotNull().height.equals(44); // includes outer padding

// Outer padding responds to taps, not just the painted part.

final buttonTopLeft = tester.getTopLeft(buttonFinder);

final buttonCenter = tester.getCenter(buttonFinder);

Or maybe what I really want here is to move renderObject and width down to where they're used.

gnprice · 2025-04-04T18:13:25Z

test/widgets/button_test.dart

+      for (double y = 0; y < 44; y++) {
+        await tester.tapAt(Offset(buttonCenter.dx, y + buttonTopLeft.dy));


nit: Perhaps this would be clearer in this form?

Suggested change

for (double y = 0; y < 44; y++) {

await tester.tapAt(Offset(buttonCenter.dx, y + buttonTopLeft.dy));

for (double y = -22; y < 22; y++) {

await tester.tapAt(buttonCenter + Offset(0, y));

gnprice · 2025-04-04T19:04:27Z

test/widgets/button_test.dart

+      check(numTapsHandled).equals(numTaps);
+
+      final textScaler = TextScaler.linear(textScaleFactorVariants.currentValue!)


I think this test would break up pretty cleanly into two tests at this spot: one checks the response to taps, and one checks the painting. That would probably make them both a little easier to read.

gnprice · 2025-04-04T19:05:33Z

lib/widgets/button.dart

+    // With [MaterialTapTargetSize.padded],
+    // make [TextButton] set 44 instead of 48 for the touch-target height.
+    final visualDensity = VisualDensity(vertical: -1);
+    // A value that [TextButton] adds to some of its layout parameters;
+    // we can cancel out those adjustments by subtracting it.
+    final densityVerticalAdjustment = visualDensity.baseSizeAdjustment.dy;


Cool, glad you found a way to control this.

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Mar 21, 2025

chrisbobbe requested a review from PIG208 March 21, 2025 22:58

chrisbobbe assigned PIG208 Mar 21, 2025

gnprice reviewed Mar 21, 2025

View reviewed changes

PIG208 reviewed Mar 25, 2025

View reviewed changes

chrisbobbe force-pushed the pr-edit-message-3 branch from 771493d to 091c102 Compare April 1, 2025 04:27

PIG208 assigned gnprice and unassigned PIG208 Apr 1, 2025

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Apr 1, 2025

PIG208 requested a review from gnprice April 1, 2025 14:54

gnprice reviewed Apr 2, 2025

View reviewed changes

chrisbobbe force-pushed the pr-edit-message-3 branch from 091c102 to 5c59015 Compare April 4, 2025 05:33

chrisbobbe added 2 commits April 4, 2025 13:38

button [nfc]: Move AnimatedScaleOnTap to new file lib/widgets/button.…

4f651e0

…dart

button: Implement ZulipWebUiKitButton, to use for edit-message UI soon

c88b217

chrisbobbe force-pushed the pr-edit-message-3 branch from 5c59015 to c88b217 Compare April 4, 2025 05:38

gnprice reviewed Apr 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit-message (3/n): Implement ZulipWebUiKitButton, for "Cancel" / "Save" buttons #1432

Edit-message (3/n): Implement ZulipWebUiKitButton, for "Cancel" / "Save" buttons #1432

chrisbobbe commented Mar 21, 2025 •

edited

Loading

gnprice Mar 21, 2025

PIG208 left a comment

PIG208 Mar 25, 2025

chrisbobbe Apr 1, 2025

PIG208 Mar 25, 2025

chrisbobbe Apr 1, 2025 •

edited

Loading

PIG208 Apr 1, 2025

PIG208 Mar 25, 2025

chrisbobbe Apr 1, 2025

PIG208 Mar 25, 2025

PIG208 Mar 25, 2025

chrisbobbe Apr 1, 2025

chrisbobbe commented Apr 1, 2025

PIG208 commented Apr 1, 2025

gnprice left a comment

gnprice Apr 2, 2025

chrisbobbe Apr 2, 2025

gnprice Apr 2, 2025

gnprice Apr 2, 2025

chrisbobbe commented Apr 4, 2025

gnprice left a comment

gnprice Apr 4, 2025

gnprice Apr 4, 2025

gnprice Apr 4, 2025

gnprice Apr 4, 2025

gnprice Apr 4, 2025

gnprice Apr 4, 2025

gnprice Apr 4, 2025

gnprice Apr 4, 2025

		required this.btnLabelAttMediumIntInfo,
		required this.btnLabelAttMediumIntDanger,

		final textScaleFactorVariants = ValueVariant(Set.of(kTextScaleFactors));
		testWidgets('button and touch-target heights', (tester) async {

		for (double y = 0; y < 44; y++) {
		await tester.tapAt(Offset(buttonCenter.dx, y + buttonTopLeft.dy));

		check(numTapsHandled).equals(numTaps);

		final textScaler = TextScaler.linear(textScaleFactorVariants.currentValue!)

Edit-message (3/n): Implement ZulipWebUiKitButton, for "Cancel" / "Save" buttons #1432

Are you sure you want to change the base?

Edit-message (3/n): Implement ZulipWebUiKitButton, for "Cancel" / "Save" buttons #1432

Conversation

chrisbobbe commented Mar 21, 2025 • edited Loading

Cancel button ("info" intent, "medium" attention)

Save button ("info" intent, "high" attention)

Choose a reason for hiding this comment

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 1, 2025

PIG208 commented Apr 1, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 4, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Mar 21, 2025 •

edited

Loading

chrisbobbe Apr 1, 2025 •

edited

Loading