RecentDmConversationsPage: Add #249

chrisbobbe · 2023-08-02T21:58:22Z

Fixes: #119

This includes my current revisions of #246, #247, and #248.

chrisbobbe · 2023-08-02T21:59:46Z

To add to the exclusions mentioned in #119 (e.g., that we don't show presence data), I should mention that this also doesn't yet show the number of unreads in each conversation, since we don't have a data structure for that yet. 🙂

chrisbobbe · 2023-08-02T23:43:01Z

Just updated with a revision that's meant to follow Vlad's design for the new screen's content area (so, excluding the app bar, for now), instead of Material Design.

chrisbobbe · 2023-08-03T00:13:41Z

(And another update with some small tweaks.)

It's meant to follow Vlad's Figma, so tagging @terpimost; feedback is appreciated. 🙂

Design things excluded here, to be done later:

The top app bar (which will take more work with navigation, etc.)
The bottom nav bar
The interactive "Message" box at the bottom
Users' presence
Conversation unread counts

Screenshot from the "Zulip test device" account:

EDIT: An earlier version of this PR and the screenshot had some of the users' avatars missing.

gnprice · 2023-08-03T00:34:59Z

Design things excluded here, to be done later:

Just to expand on this part: the reason this version leaves out the design's top and bottom app bars and "Message" box is because the various UI elements there lead to a variety of screens that aren't yet implemented. When we've implemented more of the screens, we can circle back and build a good way to navigate among them.

And the reason it leaves out the presence indicators, unread counts, ~~and some of the avatars~~, is because of various features not yet implemented on the data side. That's

presence: Presence data, and report presence #196
~~… and it looks like we don't have issues for the other two. I'll go file those.~~
… OK, unread counts: Track unread counts #253
avatars: the PR has been revised so that the avatars are good 🙂

But there are a couple of optimizations that this reminded me we haven't implemented, and I filed issues for those:
- Handle avatar-URL optimization user_avatar_url_field_optional #254
- Compute Gravatar URLs where needed (handle client_gravatar true) #255

chrisbobbe · 2023-08-03T01:20:26Z

(Updated to fill in the missing avatars.)

chrisbobbe · 2023-08-03T18:45:17Z

New screenshot from this same PR revision (ending in 4b5889c), this time with:

A conversation with many names, so they wrap
A conversation with even more names, so they wrap and get ellipsized ("…")

(These are real examples from CZO; both conversations include two users with the same name, "Chris Bobbe (Test Account)".)

chrisbobbe · 2023-08-03T20:58:13Z

Updated now that #246 is merged, and incorporating my updated revision of #247.

gnprice

Thanks for building this! Comments below.

gnprice · 2023-08-03T22:17:58Z

lib/api/route/events.dart

+    'client_gravatar': false, // TODO turn on
    'client_capabilities': {
      'notification_settings_null': true,
      'bulk_message_deletion': true,
-      'user_avatar_url_field_optional': true,
+      'user_avatar_url_field_optional': false, // TODO turn on


Let's include references to the issues (that I filed yesterday) in these TODO comments.

gnprice · 2023-08-03T22:18:38Z

lib/widgets/content.dart

+    final user = store.users[userId]!;
+
+    final resolvedUrl = switch (user.avatarUrl) {
+      null          => null, // TODO handle computing gravatars


Can also add an issue reference to this comment, while we're here.

gnprice · 2023-08-03T22:21:28Z

lib/widgets/content.dart

+/// To set the size and clip the corners to be round, pass [size].
+/// If [size] is not passed, the caller takes responsibility
+/// for doing that with its own square wrapper.


It looks like the caller that's leaving out size is doing the same thing as this would, except with a different border radius. I think it's probably cleanest for this widget to keep responsibility for applying a size and shape, and just add a double borderRadius field that callers have to specify.

Sure, I can do that.

On the other hand, that'll mean reciting the same size and border radius again for the group-DM icon/avatar, so there are two places to update if we want to change them. Or maybe I can put them in variables. 🤔

gnprice · 2023-08-03T22:23:47Z

lib/widgets/recent_dm_conversations.dart

+    final recipientsSansSelf = allRecipientIds
+      .whereNot((id) => id == selfUser.userId)
+      .map((id) => store.users[id]!)
+      .toList();


This is [DmNarrow.otherRecipientIds], right?

Ah, plus I guess it's mapping the store.users lookup. But better to push that down inside the cases, I think — that way we don't need to allocate a list in the 1:1 case.

gnprice · 2023-08-03T22:24:55Z

lib/widgets/recent_dm_conversations.dart

+        break;
+      }


no need for braces, nor break:
https://dart.dev/language/branches#switch-statements

gnprice · 2023-08-03T22:28:00Z

lib/widgets/recent_dm_conversations.dart

+      case 1: {
+        final otherUser = recipientsSansSelf.single;


Can switch on recipientsSansSelf itself rather than its length, and then this can say

case [var otherUser]:

(Or combined with my other comment above, it'd say var otherUserId and then look that up in store.users.)

gnprice · 2023-08-03T22:36:07Z

lib/widgets/recent_dm_conversations.dart

+    final selfUser = store.users[store.account.userId]!;
+    final recipientsSansSelf = allRecipientIds
+      .whereNot((id) => id == selfUser.userId)
+      .map((id) => store.users[id]!)


This ! — so that we crash if a user appears which we don't know about — reflects how we'd write this sort of logic in zulip-mobile, but I'd prefer to soften these failures while we're rewriting things here.

In particular since we're planning a product feature where guests (for example) might not know about all the users in the realm, it'll start becoming a lot more likely that the server could have a bug, or even perhaps an edge case that's hard to eliminate even when we fully understand it, where we end up with a user in a list like this that isn't a user we know about. We'll want to try to avoid those situations, but it'd be good to avoid crashing in them.

See [MessageListAppBarTitle] for an example of handling that. Basically we'll end up with things like ?? '(unknown user)' sprinkled around.

gnprice · 2023-08-03T22:41:22Z

lib/widgets/recent_dm_conversations.dart

+            child: DefaultTextStyle(
+              style: const TextStyle(
+                fontFamily: 'Source Sans 3',
+                fontSize: 17,
+                height: (20 / 17),
+                color: Color(0xFF222222),
+              ).merge(weightVariableTextStyle(context)),
+              maxLines: 2,
+              overflow: TextOverflow.ellipsis,
+              child: title),


This title widget is always a Text, and so the DefaultTextStyle is just applying to that, right?

In that case it seems more straightforward to just have the Text widget here. The switch above can compute a final String title instead of a Widget.

gnprice · 2023-08-03T22:41:50Z

lib/widgets/recent_dm_conversations.dart

+              child: title),
+          )),
+          const SizedBox(width: 8),
+          // TODO: Unread count


can include the issue reference

gnprice · 2023-08-03T22:52:29Z

lib/widgets/recent_dm_conversations.dart

+    });
+  }
+
+  Widget _buildItem(BuildContext context, DmNarrow narrow) {


I think it's probably a bit more idiomatic to break this out as its own widget.

There's some discussion of this in the StatelessWidget docs:
https://api.flutter.dev/flutter/widgets/StatelessWidget-class.html
complete with a video "Widgets vs Helper Methods". I think the reasons there why one sometimes really strongly wants separate widgets don't apply here, but it's still an indication of the direction in which to lean.

chrisbobbe · 2023-08-04T00:21:37Z

Thanks for the review! Revision pushed.

gnprice

Thanks! Comments below.

Let's also get some unit tests for this before merge. Partly that's useful because, as we learned at #219 (comment) , having some widget tests for a screen can let us know when we introduce something that causes Flex overflows or other layout errors on that screen.

Some good things to test would be:

for each of a self-1:1, another 1:1, and a group thread, test the page builds, that it has the relevant users' names, and that tapping the name navigates to the message list
have a group thread where names are long, and check the height is right and it's ellipsized (or if ellipsized is hard to check, then just make the names super long so that if it got the right height it must have cut off the text somehow, and didn't throw)
have a 1:1 thread and a group thread with unknown users, and check the phrase "unknown user" appears but mostly that it didn't throw
have 30 items in the list so it can't all fit on screen (and check the bottom item doesn't exist, just to make sure); drag to fling-scroll; check the bottom item is now visible

gnprice · 2023-08-04T20:07:01Z

lib/widgets/recent_dm_conversations.dart

+  final _avatarSize = 32.0;
+  final _avatarBorderRadius = 3.0;


Because these are the same for every widget of this type, let's make them static and const.

gnprice · 2023-08-04T20:10:17Z

lib/widgets/recent_dm_conversations.dart

+        // TODO(i18n): List formatting, like you can do in JavaScript:
+        //   new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya'])
+        //   // 'Chris、Greg、Alya'
+        title = narrow.otherRecipientIds.map((id) => store.users[id]!.fullName).join(', ');


another user-!

gnprice · 2023-08-04T20:20:38Z

lib/widgets/recent_dm_conversations.dart

+        avatar = SizedBox.square(
+          dimension: _avatarSize,
+          child: ClipRRect(
+            borderRadius: BorderRadius.all(Radius.circular(_avatarBorderRadius)),
+            clipBehavior: Clip.antiAlias,


Hmm yeah, I see, this is a bit annoying.

Still I think this is better than having Avatar sometimes take responsibility for its shape, and sometimes not. That feels like having that widget live at two different conceptual layers depending on the arguments it's constructed with.

I think the Flutter-upstream-style solution here would probably be to decompose Avatar into two widgets:

one named like AvatarImage that just builds the RealmContentNetworkImage (consulting the store in order to do so);

one named like AvatarShape that is very much like the _AvatarWrapper in your previous revision, but taking size and borderRadius as parameters;

and then also perhaps one named Avatar that is just a convenience composing those two.

Then this switch would compute a Widget avatar that's either an AvatarImage or an Icon, and then in a single spot afterward this build method would enclose that in an AvatarShape.

That makes sense! I can go for that in my next revision.

gnprice · 2023-08-04T20:26:08Z

lib/widgets/recent_dm_conversations.dart

+            child: Text(
+              title,


nit: I think the natural analogue of putting a widget's child last is to put a Text widget's positional String argument last.

This only became possible with a relatively recent version of Dart, circa 2022, so there may be a lot of code even in the Flutter tree that doesn't. But I think it's the natural thing to do now that one can.

gnprice · 2023-08-04T20:32:21Z

lib/widgets/content.dart

+    super.key,
+    required this.userId,
+    required this.size,
+    this.borderRadius = 4, // TODO vary default value with [size]?


I think probably actually cleanest to make this argument required too. That way the value chosen for it is always evident in the source right next to the chosen size.

gnprice · 2023-08-04T20:32:26Z

lib/widgets/content.dart

+    super.key,
+    required this.userId,
+    required this.size,
+    this.borderRadius = 4, // TODO vary default value with [size]?


I think probably actually cleanest to make this argument required too. That way the value chosen for it is always evident in the source right next to the chosen size.

As Greg suggests: zulip#249 (comment)

Discussion: zulip#249 (comment)

chrisbobbe · 2023-08-08T21:04:47Z

Thanks for the review! Revision pushed, with a first stab at all those unit tests you mentioned.

Some things to flag as maybe non-optimal:

Setting the test device size doesn't seem to do anything when running with flutter run vs. flutter test: Inconsistent sizes reported when running a test with flutter run vs flutter test flutter/flutter#124071 , so with flutter run some tests fail. This problem also affects existing tests in test/widgets/message_list_test.dart.
Possible over-reliance on implementation details, like checking specifically for RecentDmConversationsItem, AvatarShape, AvatarImage, and Text. (?)

To check for correct navigation on tapping an item, I changed application code, to add an identifying RouteSettings to the message-list route:

class MessageListPage extends StatefulWidget {
  const MessageListPage({super.key, required this.narrow});

  static Route<void> buildRoute({required BuildContext context, required Narrow narrow}) {
    return MaterialAccountPageRoute(context: context,
      settings: RouteSettings(name: 'message_list', arguments: narrow), // for testing
      builder: (context) => MessageListPage(narrow: narrow));
  }

As discussed, I didn't find an ideal way to check truncation of the items' titles, for example by checking for the position of a "…" character. Here, I just make the title super-long and assume that it must have been truncated properly if the text doesn't exceed two lines.

As Greg suggests: zulip#249 (comment)

Discussion: zulip#249 (comment)

chrisbobbe · 2023-08-09T19:45:25Z

(Just rebased.)

gnprice

Thanks! Generally looks great; a few comments below.

Setting the test device size doesn't seem to do anything when running with flutter run vs. flutter test:

(See reply in an inline comment below.)

Possible over-reliance on implementation details, like checking specifically for RecentDmConversationsItem, AvatarShape, AvatarImage, and Text. (?)

I think these are fine, except the inspection of Text can be made more flexible using find.text instead as described in a comment below.

In particular RecentDmConversationsItem reflects the fact that this screen fundamentally is going to be some kind of list — its job is to show a list of the recent DM conversations — and so we're naturally going to have some kind of widget for an item in the list. The name of that widget type, or its exact interface, could change, so there's a bit of exposure here to having to change the tests in the event of a refactor, but I don't think it's too bad.

To check for correct navigation on tapping an item, I changed application code, to add an identifying RouteSettings to the message-list route:

Yeah, I think editing that application code (to make the target page more inspectable, where currently it's expressed only in the form of a builder function) is quite reasonable. I don't love the details of the RouteSettings solution, but can experiment with alternatives post-merge as discussed below.

As discussed, I didn't find an ideal way to check truncation of the items' titles, for example by checking for the position of a "…" character. Here, I just make the title super-long and assume that it must have been truncated properly if the text doesn't exceed two lines.

👍

gnprice · 2023-08-09T20:53:25Z

lib/widgets/content.dart

+    if (user == null) {
+      // TODO(log) log?


Suggested change

if (user == null) {

// TODO(log) log?

if (user == null) { // TODO(log)

Makes it a bit easier to see what the TODO is about when doing a grep.

gnprice · 2023-08-09T20:53:38Z

test/widgets/recent_dm_conversations_test.dart

+import 'content_checks.dart';
+
+
+Future<void> setupPage(WidgetTester tester, {


nit:

Suggested change

import 'content_checks.dart';

Future<void> setupPage(WidgetTester tester, {

import 'content_checks.dart';

Future<void> setupPage(WidgetTester tester, {

gnprice · 2023-08-09T20:56:06Z

lib/widgets/content.dart

+    super.key,
+    required this.userId,
+    required this.size,
+    required this.borderRadius,


The required borderRadius can be squashed into the "Pull out Avatar widget" commit, resolving the TODO in the latter.

gnprice · 2023-08-09T21:14:05Z

test/widgets/recent_dm_conversations_test.dart

+        check(shape)
+          ..size.equals(32)
+          ..borderRadius.equals(3);


I'm lukewarm on checking this level of detail of the layout. It's the sort of test that's liable to end up acting just as another place we have to update when we make changes.

But because it's nicely centralized in this test helper, it is at least just one additional place to update, so it's a lot cheaper than the situation people often get into where tons of tests need to be updated for a small change in the code under test. So it's fine.

Makes sense. I'm not attached to it; I just saw it as an opportunity to be thorough. 🙂 I see what you mean about how this could get annoying if it weren't centralized in this helper.

gnprice · 2023-08-09T21:17:02Z

test/widgets/recent_dm_conversations_test.dart

+          await setupPage(tester, users: [], dmMessages: [message]);
+
+          checkAvatar(tester, DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId));
+          check(getTitle(tester).data).equals(eg.selfUser.fullName);


Instead of find.byType(Text) and this check on Text.data, this can use find.text.

That's a bit more flexible because it will also match if there's inline formatting on the text, or even if there isn't but the code uses Text.rich anyway (perhaps as part of some refactoring where the same code sometimes does apply inline formatting).

gnprice · 2023-08-09T21:21:51Z

test/widgets/recent_dm_conversations_test.dart

+      void checkTitleHeight(WidgetTester tester, int expectedLines) {
+        final renderObject = tester.renderObject<RenderParagraph>(find.byWidget(getTitle(tester)));
+        check(renderObject.size.height).equals(
+          20.0 // line height
+          * expectedLines);


Ah, sorry, I was ambiguous when I said "check the height is right" above. I really just meant checking that the height of the whole item is right — that it's still 48, confirming that it hasn't been blown up taller to make room for lots of names.

This is fine now that it's done; it's just more work than I intended.

No problem 🙂—in fact I think this way makes it simpler to reason about what might be causing the height to blow up, whether it's that the truncation failed, or the system font size was increased (or possibly something else I'm not thinking of?). With a check on the whole item, we'd also have to think about the 4dp of vertical padding.

gnprice · 2023-08-09T21:41:15Z

lib/widgets/message_list.dart

    return MaterialAccountPageRoute(context: context,
+      settings: RouteSettings(name: 'message_list', arguments: narrow), // for testing
      builder: (context) => MessageListPage(narrow: narrow));


Hmm, I see.

This RouteSettings and the builder feel like they're expressing the same information twice. I'll try experimenting with ways to deduplicate that, and also to avoid us having to make up names like message_list for each of the pages we might want to test something navigating to. But that can be after merging this PR.

Interesting, yeah. Thanks!

gnprice · 2023-08-09T21:45:38Z

test/widgets/recent_dm_conversations_test.dart

+        final pushedRoutes = <Route<dynamic>>[];
+        final testNavObserver = TestNavigatorObserver()
+          ..onPushed = (route, prevRoute) => pushedRoutes.add(route);


gnprice · 2023-08-09T21:46:19Z

test/test_navigation.dart

+// Inspired by test code in the Flutter tree:
+//   https://github.com/flutter/flutter/blob/53082f65b/packages/flutter/test/widgets/observer_tester.dart
+//   https://github.com/flutter/flutter/blob/53082f65b/packages/flutter/test/widgets/navigator_test.dart


Cool, good find.

gnprice · 2023-08-09T21:58:32Z

test/widgets/recent_dm_conversations_test.dart

+
+  // N.B. this doesn't seem to work when running the tests with `flutter run`
+  // instead of `flutter test`, resulting in test failures. Upstream issue:
+  //   https://github.com/flutter/flutter/issues/124071
+  tester.view.physicalSize = const Size(390, 844) * 3; // iPhone 13 Pro


That upstream issue is equally applicable if we don't set physicalSize ourselves, right? It does seem like an annoying issue. (I see in fact I'd already thumbs-upped it.)

I think it's a bit cleaner if we can avoid setting the physical size when there isn't a reason the tests specifically need to interact with it, and write the tests so that they work on the default flutter_test screen size. In particular, if all the tests use the same screen size except when the test case specifically mentions a different one, that makes it easier to learn a mental model of the size of it so that one can look at a given test and mentally picture how it's likely to lay out.

That default size is 800x600 logical pixels, so rather wider and a bit shorter than this. (I believe this line is written with the assumption of a 3x device pixel ratio, so that 390x844 is the size it gives in logical pixels.) For these tests, I think that means using longer names in the "long names" tests, and possibly no other changes.

One takeaway from that upstream issue is that it's another reason to keep the app working reasonably on desktop. As the upstream report shows, on desktop the issue has no effect on the reported size — it can affect the reported DPR and reported size in physical pixels, but those are something that far less code interacts with and so will have no effect at all on most tests. So that makes desktop a good target when using flutter run to investigate a test.

(I also left a comment on the upstream issue, because I think the original report didn't capture the full scope of the impact.)

As the upstream report shows, on desktop the issue has no effect on the reported size

This doesn't seem to hold in my environment: flutter/flutter#124071 (comment) and with this size-setting line removed, the "long name" tests are still giving different results for flutter test and flutter run. 😵

Awkward.

The thing is that this size-setting line isn't a great solution either — it's only helpful if one has an iPhone 13 Pro, or something with the same screen size.

(Or I guess if one is working on a Mac and can run a simulated iPhone 13 Pro. Though even then one might have wanted to run it on a physical device.)

I think if you make the name lists in the "very long name" tests sufficiently long, then they should work on a wide range of sizes including the default and any likely devices.

The intermediate-length "long name" tests are trickier, if they're trying to target reaching two lines and no longer. But as it is, it seems like they don't have a way to check that anyway — their checks are exactly the same as the "very long name" tests. So I think the "short name" and "very long name" versions together give us essentially all the same useful coverage.

As Greg suggests: zulip#249 (comment)

Discussion: zulip#249 (comment)

chrisbobbe · 2023-08-10T01:31:05Z

Thanks for the review! Revision pushed.

gnprice · 2023-08-10T04:36:38Z

Thanks for the revision!

This all LGTM. I think the intermediate-length "long name" tests may not be adding anything for us over the "short name" and "very long name" tests:
#249 (comment)
Please go ahead and merge after either removing those tests if you agree, or keeping them if you think they're useful.

chrisbobbe · 2023-08-10T17:08:09Z

Yeah, agreed. Thanks for the many reviews! Merging, with those specific tests removed.

Made by Vlad for mobile. This is the SVG file Vlad shared on CZO: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20DM-conversation.20list/near/1594671 Related: zulip#119

…ration (And, since the linter complains of sized_box_for_whitespace <https://dart.dev/tools/linter-rules/sized_box_for_whitespace> , swap out the Container for a SizedBox.)

Assets and license downloaded from https://github.com/adobe-fonts/source-sans/tree/ed1808970 , so it's version 3.052.

We should implement these optimizations soon, like zulip-mobile does, but the new "Direct messages" screen (coming next) would otherwise be missing lots of avatars, which looks awkward.

As Greg suggests: zulip#249 (comment)

Discussion: zulip#249 (comment)

…ting This way, the Route object itself is distinguishable from other Route objects. This will come in handy in widget tests for the upcoming recent-DMs screen (zulip#119), where we'll want to assert that tapping on a conversation brings up the appropriate message list. While we're at it, add the usual `extension FooChecks on Subject<Foo>` boilerplate that we'll use in those tests.

The screen's content area (so, the list of conversations, but not the app bar at the top) is built against Vlad's design: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20DM-conversation.20list/near/1594654 except that some features that appear in that design are left unimplemented for now, since we don't have data structures for them yet: - unread counts - user presence Fixes: zulip#119

As discussed when we added this first test of this kind: zulip#249 (comment)

As discussed when we added this first test of this kind: #249 (comment)

I missed this in zulip#249 and made the rows transparent, oops. So they were taking on the scaffold background color, 0xfffafafa a.k.a. ThemeData.canvasColor. (If we were using Material 3, the scaffold background color would be 0xfffefbff a.k.a. ThemeData.colorScheme.background. Neither color is correct for what the Figma actually specifies for the surface underneath this screen's scrollable content. That's 0xfff6f6f6, and we'll start using it soon.) The ink effect on touch was being enabled by the scaffold's underlying Material. To preserve the ink effect, use a Material here instead of e.g. a ColoredBox. See the design in Figma: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=341%3A12378&mode=dev

chrisbobbe requested a review from gnprice August 2, 2023 21:58

chrisbobbe force-pushed the pr-recent-dm-conversations-ui branch from f02d4e3 to 257af55 Compare August 2, 2023 23:41

chrisbobbe force-pushed the pr-recent-dm-conversations-ui branch from 257af55 to bc361d8 Compare August 3, 2023 00:07

gnprice mentioned this pull request Aug 3, 2023

icons: Add group_dm custom icon, for recent-DMs screen #248

Closed

chrisbobbe force-pushed the pr-recent-dm-conversations-ui branch from bc361d8 to 4b5889c Compare August 3, 2023 01:19

chrisbobbe force-pushed the pr-recent-dm-conversations-ui branch from 4b5889c to caeca7e Compare August 3, 2023 20:57

gnprice reviewed Aug 3, 2023

View reviewed changes

chrisbobbe force-pushed the pr-recent-dm-conversations-ui branch from caeca7e to 4de84ae Compare August 4, 2023 00:21

gnprice reviewed Aug 4, 2023

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Aug 8, 2023

content [nfc]: Decompose Avatar into AvatarImage and AvatarShape

fb4f9f8

As Greg suggests: zulip#249 (comment)

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Aug 8, 2023

content: Have AvatarImage handle a missing user

73d6aa3

Discussion: zulip#249 (comment)

chrisbobbe force-pushed the pr-recent-dm-conversations-ui branch from 4de84ae to c79cd7c Compare August 8, 2023 20:58

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Aug 9, 2023

content [nfc]: Decompose Avatar into AvatarImage and AvatarShape

46b4eb7

As Greg suggests: zulip#249 (comment)

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Aug 9, 2023

content: Have AvatarImage handle a missing user

40bc031

Discussion: zulip#249 (comment)

chrisbobbe force-pushed the pr-recent-dm-conversations-ui branch from c79cd7c to 0b71040 Compare August 9, 2023 19:45

gnprice reviewed Aug 9, 2023

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Aug 10, 2023

content [nfc]: Decompose Avatar into AvatarImage and AvatarShape

b9694e3

As Greg suggests: zulip#249 (comment)

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Aug 10, 2023

content: Have AvatarImage handle a missing user

701323c

Discussion: zulip#249 (comment)

chrisbobbe force-pushed the pr-recent-dm-conversations-ui branch from 0b71040 to 63f695f Compare August 10, 2023 01:31

chrisbobbe force-pushed the pr-recent-dm-conversations-ui branch from 63f695f to 80c725e Compare August 10, 2023 17:07

chrisbobbe added 11 commits August 10, 2023 10:09

icons: Add group_dm custom icon, for recent-DMs screen

845987f

Made by Vlad for mobile. This is the SVG file Vlad shared on CZO: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20DM-conversation.20list/near/1594671 Related: zulip#119

content [nfc]: Pull out Avatar widget

065cd4f

content [nfc]: Use ClipRRect instead of Container's clipBehavior/deco…

be47daa

…ration (And, since the linter complains of sized_box_for_whitespace <https://dart.dev/tools/linter-rules/sized_box_for_whitespace> , swap out the Container for a SizedBox.)

content [nfc]: In Avatar, use SizedBox.square constructor

95b29e5

content [nfc]: Refactor Avatar a bit

6e1c1ad

text: Bundle 'Source Sans 3' font into the app

a6692c7

Assets and license downloaded from https://github.com/adobe-fonts/source-sans/tree/ed1808970 , so it's version 3.052.

register: Temporarily ask the server to send all avatar URLs

caa9603

We should implement these optimizations soon, like zulip-mobile does, but the new "Direct messages" screen (coming next) would otherwise be missing lots of avatars, which looks awkward.

content [nfc]: Decompose Avatar into AvatarImage and AvatarShape

c75b6ae

As Greg suggests: zulip#249 (comment)

content: Have AvatarImage handle a missing user

dc1502d

Discussion: zulip#249 (comment)

chrisbobbe force-pushed the pr-recent-dm-conversations-ui branch from 80c725e to aa905d7 Compare August 10, 2023 17:09

chrisbobbe merged commit aa905d7 into zulip:main Aug 10, 2023

chrisbobbe deleted the pr-recent-dm-conversations-ui branch August 10, 2023 17:19

gnprice added a commit to gnprice/zulip-flutter that referenced this pull request Aug 14, 2023

nav [nfc]: Make navigation more cleanly testable, with WidgetRoute

6865dbd

As discussed when we added this first test of this kind: zulip#249 (comment)

gnprice mentioned this pull request Aug 14, 2023

nav [nfc]: Make navigation more cleanly testable, with WidgetRoute #270

Merged

chrisbobbe pushed a commit that referenced this pull request Aug 15, 2023

nav [nfc]: Make navigation more cleanly testable, with WidgetRoute

1ff9acf

As discussed when we added this first test of this kind: #249 (comment)

This was referenced Sep 23, 2023

Handle avatar-URL optimization user_avatar_url_field_optional #254

Open

Compute Gravatar URLs where needed (handle client_gravatar true) #255

Open

	if (user == null) {
	// TODO(log) log?
	if (user == null) { // TODO(log)

		import 'content_checks.dart';


		Future<void> setupPage(WidgetTester tester, {

RecentDmConversationsPage: Add #249

RecentDmConversationsPage: Add #249

Conversation

chrisbobbe commented Aug 2, 2023

chrisbobbe commented Aug 2, 2023

chrisbobbe commented Aug 2, 2023

chrisbobbe commented Aug 3, 2023 • edited Loading

gnprice commented Aug 3, 2023 • edited Loading

chrisbobbe commented Aug 3, 2023

chrisbobbe commented Aug 3, 2023

chrisbobbe commented Aug 3, 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

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

Choose a reason for hiding this comment

chrisbobbe commented Aug 4, 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

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 Aug 8, 2023 • edited Loading

chrisbobbe commented Aug 9, 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

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

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 Aug 10, 2023

gnprice commented Aug 10, 2023

chrisbobbe commented Aug 10, 2023

chrisbobbe commented Aug 3, 2023 •

edited

Loading

gnprice commented Aug 3, 2023 •

edited

Loading

chrisbobbe commented Aug 8, 2023 •

edited

Loading