compose_box [nfc]: Add InsetShadowBox #990

PIG208 · 2024-10-08T00:53:21Z

This casts a static shadow on top of a child widget from its top edge and bottom edge.

To review its effect, see #928. This is intended to prep for #853.

chrisbobbe

Thanks! Small comments below, and I'll mark for Greg's review.

chrisbobbe · 2024-10-08T19:04:26Z

lib/widgets/inset_shadow_box.dart

@@ -0,0 +1,45 @@
+import 'package:flutter/material.dart';
+
+/// A widget that overlays inset shadows on a child.


Could you expand on this a little bit, maybe mentioning the scroll-through-shadow use case?

There's a similar-sounding effect that this looks like it might describe, but it turns out it doesn't; that's box-shadow: inset. It would be nice to have a full implementation of box-shadow: inset—we have a TODO in the reaction-chip UI:

// TODO shadow effect, following web, which uses `box-shadow: inset`: // https://developer.mozilla.org/en-US/docs/Web/CSS/box-shadow#inset // Needs Flutter support for something like that: // https://github.com/flutter/flutter/issues/18636 // https://github.com/flutter/flutter/issues/52999 // Until then use a solid color; a much-lightened version of the shadow color. // Also adapt by making [borderUnselected] more transparent, so we'll // want to check that against web when implementing the shadow. bgUnselected: const HSLColor.fromAHSL(0.08, 210, 0.50, 0.875).toColor(),

—but that's not what InsetShadowBox is. 🙂 (InsetShadowBox wouldn't handle the "stadium" shape of the reaction chips, for example.)

Makes sense. I think this means that we should rename this widget to RectangularInsetShadowBox, to make it clearer.

chrisbobbe · 2024-10-08T19:05:23Z

lib/widgets/inset_shadow_box.dart

+  Widget build(BuildContext context) {
+    return Stack(
+        children: [
+          child,
+          Positioned(top: 0, left: 0, right: 0,
+            child: Container(height: top, decoration: _shadowFrom(Alignment.topCenter))),
+          Positioned(bottom: 0, left: 0, right: 0,
+            child: Container(height: bottom, decoration: _shadowFrom(Alignment.bottomCenter))),
+        ]);
+  }


Suggested change

Widget build(BuildContext context) {

return Stack(

children: [

child,

Positioned(top: 0, left: 0, right: 0,

child: Container(height: top, decoration: _shadowFrom(Alignment.topCenter))),

Positioned(bottom: 0, left: 0, right: 0,

child: Container(height: bottom, decoration: _shadowFrom(Alignment.bottomCenter))),

]);

}

Widget build(BuildContext context) {

return Stack(children: [

child,

Positioned(top: 0, left: 0, right: 0,

child: Container(height: top, decoration: _shadowFrom(Alignment.topCenter))),

Positioned(bottom: 0, left: 0, right: 0,

child: Container(height: bottom, decoration: _shadowFrom(Alignment.bottomCenter))),

]);

}

chrisbobbe · 2024-10-08T19:09:23Z

lib/widgets/inset_shadow_box.dart

@@ -0,0 +1,45 @@
+import 'package:flutter/material.dart';


nit:

Suggested change

import 'package:flutter/material.dart';

import 'package:flutter/widgets.dart';

(not sure how much we care about being agnostic about Material though.)

gnprice

Thanks @PIG208, and thanks @chrisbobbe for the previous review!

Generally this looks great. A few nits below.

Let's also have a test or two.

One thing to test is that the shadows don't add padding — so e.g. a child with height 40 and shadows of height 8 produce height 40, not 56.
Ideally I'd like to have a test for the widget's actual main functionality too. I'm not sure there's a good way to do that without more work than it's worth, though.
- One way would be with golden images. That seems like definitely more work than it's worth here.
- The other possible way would be a painting test. That'd be worth spending like 15 minutes investigating to try to get to work — it might not be hard.

gnprice · 2024-10-08T20:21:27Z

lib/widgets/rectangular_inset_shadow_box.dart

+      Positioned(top: 0, left: 0, right: 0,
+        child: Container(height: top, decoration: _shadowFrom(Alignment.topCenter))),


nit: Positioned can control the height, right?

Suggested change

Positioned(top: 0, left: 0, right: 0,

child: Container(height: top, decoration: _shadowFrom(Alignment.topCenter))),

Positioned(top: 0, height: top, left: 0, right: 0,

child: Container(decoration: _shadowFrom(Alignment.topCenter))),

Then also Container can I think be reduced to DecoratedBox.

gnprice · 2024-10-08T20:31:00Z

lib/widgets/rectangular_inset_shadow_box.dart

+///
+/// See also:
+///  * https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3860-11890&node-type=frame&t=oOVTdwGZgtvKv9i8-0
+class RectangularInsetShadowBox extends StatelessWidget {


"rectangular box" seems redundant — a box is necessarily rectangular. I think InsetShadowBox would be a fine name.

Or I guess this is a box with rectangular inset shadows (the shadows are rectangular). But I think an "inset shadow" on a box, which is a rectangle, is also going to be rectangular.

It's true this doesn't cover as wide a range of cases as CSS box-shadow: inset, as discussed at #990 (comment) . I think that reflects the CSS feature growing to be wider than its name fits. To clarify that this doesn't cover that CSS feature, the clearest thing would be to just mention it in the doc.

gnprice · 2024-10-08T20:33:00Z

lib/widgets/rectangular_inset_shadow_box.dart

@@ -0,0 +1,51 @@
+import 'package:flutter/widgets.dart';


nit: generally we've followed the Flutter upstream tree in grouping related functionality together into source files, rather than atomizing each class into its own tiny file.

One consequence is that the file names are shorter and more general than the class names.

Here there might not be any existing other code for this to share a file with, but we can still give it a bit of a broader, shorter name. Say inset_shadow.dart.

PIG208 · 2024-10-09T00:50:43Z

Updated the PR with tests and fixes. This is ready for review!

gnprice

Thanks for the revision! Comments below.

gnprice · 2024-10-10T00:41:19Z

test/widgets/inset_shadow.dart

+
+import '../flutter_checks.dart';
+
+void main() {


This test doesn't actually run in flutter test (or tools/check) because the name doesn't end with _test.dart 🙂

gnprice · 2024-10-10T00:42:12Z

test/widgets/inset_shadow.dart

+      return (Symbol methodName, List<dynamic> arguments) {
+        check(methodName).equals(#drawRect);
+        check(arguments[0]).isA<Rect>().equals(rect);
+        check(arguments[1]).isA<Paint>().shader.isA<ui.Gradient>();


Worth a one-line comment saying that Gradient is opaque and so this is the most we can check about it.

gnprice · 2024-10-10T00:46:40Z

test/widgets/inset_shadow.dart

+        child: SizedBox(width: 20, height: 20,
+          child: InsetShadowBox(top: 7, bottom: 3,
+            color: Colors.red,
+            child: Placeholder())))));
+
+    final childRect = tester.getRect(find.byType(Placeholder));
+    check(childRect).equals(const Rect.fromLTRB(0, 0, 20, 20));


I believe this check would pass no matter what InsetShadowBox attempted to do for its layout. Its parent (the SizedBox) has decreed exactly what its size is, so it's not able to be any other size. See:
https://docs.flutter.dev/get-started/fundamentals/layout#constraints
https://api.flutter.dev/flutter/rendering/RenderBox-class.html#layout

It's worth trying to verify that — edit InsetShadowBox so that it does use top and bottom as padding, and see if this test still passes. I believe it will.

To make the test effective, I think what's needed is to have a SizedBox as the child, not the parent. Then do confirm that that test fails if you make that change to InsetShadowBox.

We actually caught a bug with the implementation that allows InsetShadowBox to modify the constraints from the parent. The test is updated to catch layout changes.

gnprice · 2024-10-10T00:47:59Z

test/widgets/inset_shadow.dart

+        child: SizedBox(width: 20, height: 20,
+          child: InsetShadowBox(top: 7, bottom: 3,
+            color: Colors.red,
+            child: Placeholder())))));


In tests, typically preferable to say something like SizedBox.shrink() instead of Placeholder. The Placeholder widget has a more complicated behavior that's useful for making it visible in development, but in a test makes the behavior more complex to reason through.

This casts fixed size shadows on top of a child widget from its top edge and bottom edge. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

gnprice · 2024-10-11T00:15:51Z

Thanks for the revision! Looks good; merging.

PIG208 mentioned this pull request Oct 8, 2024

action_sheet: Redesign bottom sheet #853

Merged

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Oct 8, 2024

PIG208 assigned chrisbobbe Oct 8, 2024

PIG208 requested a review from chrisbobbe October 8, 2024 00:54

PIG208 force-pushed the pr-inset-shadow branch from 490c274 to 9390c9d Compare October 8, 2024 01:07

chrisbobbe reviewed Oct 8, 2024

View reviewed changes

chrisbobbe 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 Oct 8, 2024

chrisbobbe assigned gnprice and unassigned chrisbobbe Oct 8, 2024

chrisbobbe requested a review from gnprice October 8, 2024 19:10

PIG208 force-pushed the pr-inset-shadow branch 3 times, most recently from 978c9be to 8fd9107 Compare October 8, 2024 19:41

gnprice reviewed Oct 8, 2024

View reviewed changes

PIG208 force-pushed the pr-inset-shadow branch from 8fd9107 to d25c594 Compare October 9, 2024 00:31

PIG208 requested a review from gnprice October 9, 2024 00:50

gnprice reviewed Oct 10, 2024

View reviewed changes

PIG208 force-pushed the pr-inset-shadow branch from d25c594 to fb1ef14 Compare October 10, 2024 19:56

compose_box [nfc]: Add InsetShadowBox

a1b5579

This casts fixed size shadows on top of a child widget from its top edge and bottom edge. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

gnprice force-pushed the pr-inset-shadow branch from fb1ef14 to a1b5579 Compare October 11, 2024 00:15

gnprice merged commit a1b5579 into zulip:main Oct 11, 2024

PIG208 deleted the pr-inset-shadow branch October 11, 2024 00:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose_box [nfc]: Add InsetShadowBox #990

compose_box [nfc]: Add InsetShadowBox #990

PIG208 commented Oct 8, 2024

chrisbobbe left a comment

chrisbobbe Oct 8, 2024

PIG208 Oct 8, 2024

chrisbobbe Oct 8, 2024

chrisbobbe Oct 8, 2024

gnprice left a comment

gnprice Oct 8, 2024

gnprice Oct 8, 2024

gnprice Oct 8, 2024

PIG208 commented Oct 9, 2024 •

edited

Loading

gnprice left a comment

gnprice Oct 10, 2024

gnprice Oct 10, 2024

gnprice Oct 10, 2024

PIG208 Oct 10, 2024

gnprice Oct 10, 2024

gnprice commented Oct 11, 2024

		@@ -0,0 +1,45 @@
		import 'package:flutter/material.dart';

		/// A widget that overlays inset shadows on a child.

	import 'package:flutter/material.dart';
	import 'package:flutter/widgets.dart';

		Positioned(top: 0, left: 0, right: 0,
		child: Container(height: top, decoration: _shadowFrom(Alignment.topCenter))),

compose_box [nfc]: Add InsetShadowBox #990

compose_box [nfc]: Add InsetShadowBox #990

Conversation

PIG208 commented Oct 8, 2024

chrisbobbe 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

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

PIG208 commented Oct 9, 2024 • edited Loading

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

gnprice commented Oct 11, 2024

PIG208 commented Oct 9, 2024 •

edited

Loading