Skip to content

ui: Fix some design deviations in the recent DMs page; migrate to Material Design 3 #380

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
Merged
merged 4 commits into from
Nov 16, 2023
Merged

ui: Fix some design deviations in the recent DMs page; migrate to Material Design 3 #380

merged 4 commits into from
Nov 16, 2023

Conversation

chrisbobbe
Copy link
Collaborator

@chrisbobbe chrisbobbe commented Nov 13, 2023 •
edited
Loading

Doing our planned M3 migration (#225) gives us a convenient pattern that I'm hoping will make it easier to follow the Figma spec in a certain way. Details in the last commit.

Before After
image image

Fixes: #225

@chrisbobbe chrisbobbe requested a review from gnprice November 13, 2023 05:10
@chrisbobbe chrisbobbe mentioned this pull request Nov 13, 2023
Merged
@gnprice
Copy link
Member

gnprice commented Nov 13, 2023

Thanks for taking care of this!

In the message list, it looks like this increases the line-height of all the text in message content. Let's adjust to keep that unchanged.

Everything else LGTM.

@gnprice gnprice mentioned this pull request Nov 13, 2023
Closed
7 tasks
@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Nov 15, 2023 •
edited
Loading

In the message list, it looks like this increases the line-height of all the text in message content. Let's adjust to keep that unchanged.

I believe:

  • In main (with M2), the effective TextStyle.height is null.
  • In this PR, it's 1.43.

(In Flutter's lib/src/material/typography.dart, see englishLike2014.bodyMedium and _M3Typography.englishLike.bodyMedium.)

Do you know a good way to clobber the 1.43 with null? There's no numeric value (such as 1) that's equivalent to null, it seems, from reading the doc.

@gnprice
Copy link
Member

gnprice commented Nov 15, 2023

Hmm, I guess TextStyle.copyWith doesn't work because there's no way to express "set this to null" — it just looks like "leave this unchanged".

(This is the problem that Drift solves with the Value type, as seen on our AccountsCompanion class.)

Well… these values aren't going to change upstream, unless there's some kind of bug in them. (Based on the pattern to date, they'll keep compatibility and make new names for the next Material version.) So we could just copy them from upstream, and delete that one bit. Ugly and certainly wants a comment saying where the values are copied from, but it'll work.

@gnprice
Copy link
Member

gnprice commented Nov 15, 2023

Alternatively, for any given font there is a numeric value equivalent to null:

When height is null or omitted, the line height will be determined by the font's metrics directly, which may differ from the fontSize.

So since we're using a particular font, we could determine that value and use it.

@chrisbobbe
Copy link
Collaborator Author

So since we're using a particular font, we could determine that value and use it.

Hmm yeah, that seems right. We're not yet using a particular font; we have #294 for that. But we plan to.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Nov 15, 2023
@chrisbobbe
content: Follow web app in using Source Sans 3, size 14, height 17
b8a6414 
From looking at the "Computed" tab in the Chrome DevTools element
inspector, on CZO at 8.0-dev-2850-gef1c1ce05f.

Technically the web app has a computed `line-height` of 16.996px,
but that seems like a rounding error.

One reason for doing this now is to opt out of a default that would
take effect with the upcoming migration to Material 3. The text
would otherwise get a line height of 1.43 times the font size, and
we want it to be denser than that. We briefly explored how to
preserve the line height exactly, across the M3 migration, but the
solutions we found seemed more awkward than just taking care of
this:
  zulip#380 (comment)

Fixes-partly: zulip#294
@chrisbobbe chrisbobbe force-pushed the pr-m3-and-recent-dms-page-fixes branch from 2e72a32 to 8863fda Compare November 15, 2023 17:14
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Nov 15, 2023
@chrisbobbe
content: Follow web app in using Source Sans 3, size 14, height 17
2f1e16e 
Values obtained by looking at the "Computed" tab in the Chrome
DevTools element inspector, on CZO at 8.0-dev-2850-gef1c1ce05f.

Technically the web app has a computed `line-height` of 16.996px,
but that seems like a rounding error.

One reason for doing this now is to opt out of a default that would
take effect with the upcoming migration to Material 3. The text
would otherwise get a line height of 1.43 times the font size, and
we want it to be denser than that. We briefly explored how to
preserve the line height exactly, across the M3 migration, but the
solutions we found seemed more awkward than just taking care of
this now:
  zulip#380 (comment)

Fixes-partly: zulip#157
Fixes-partly: zulip#294
@chrisbobbe chrisbobbe force-pushed the pr-m3-and-recent-dms-page-fixes branch from 8863fda to 0ad5005 Compare November 15, 2023 17:20
@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Nov 15, 2023 •
edited
Loading

Thanks for the review! Revision pushed. I've found a way to make message content at least as dense vertically as before. It's not as minimal a change as you might've had in mind for the M3 migration, but I think it's in the right direction, because it mimics the web app more closely. And it avoids the awkwardnesses of an approach like reciting lots of details from the Typography API. And I couldn't easily find out how to do this:

So since we're using a particular font, we could determine that value […].

Before After
image image

@gnprice
Copy link
Member

gnprice commented Nov 15, 2023

Cool, that solution SGTM. Would you rebase this onto main?

I kind of think I like that "before" text better than the "after". But we'll see how we feel after using the new for a bit, and in any case that's a discussion to have separately.

@chrisbobbe
recent_dm_conversations: Make rows white, like in Figma
b3551e6 
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
content: Follow web app in using Source Sans 3, size 14, height 17
c7be9c3 
Values obtained by looking at the "Computed" tab in the Chrome
DevTools element inspector, on CZO at 8.0-dev-2850-gef1c1ce05f.

Technically the web app has a computed `line-height` of 16.996px,
but that seems like a rounding error.

One reason for doing this now is to opt out of a default that would
take effect with the upcoming migration to Material 3. The text
would otherwise get a line height of 1.43 times the font size, and
we want it to be denser than that. We briefly explored how to
preserve the line height exactly, across the M3 migration, but the
solutions we found seemed more awkward than just taking care of
this now:
  zulip#380 (comment)

Fixes-partly: zulip#157
Fixes-partly: zulip#294
@chrisbobbe
ui: Migrate to Material Design 3
2f01c51 
This completes the checklist at zulip#225. I tested the screen-reader
interface change with VoiceOver on my iPhone.

Fixes: zulip#225
@chrisbobbe
ui: Use 0xfff6f6f6 for ColorScheme.background, following Figma
21dbae1 
From logging, this would otherwise be 0xfffefbff.

See the Flutter doc on ColorScheme.background:
  https://api.flutter.dev/flutter/material/ColorScheme/background.html

It simply says:
> A color that typically appears behind scrollable content.

This change is definitely the boldest we've been in perceiving a
pattern in the Figma spec and organizing our code to fit it. We
should cheerfully do something else if it turns out to make it
harder instead of easier to follow the spec.

Prompted by noticing that the surface underneath the scrollable
content in RecentDmConversationsPage wasn't colored according to the
Figma. And that the Figma's design for the Inbox page (zulip#117) had the
same color for the surface underneath *its* scrollable content. This
change fixes the RecentDmConversationsPage and makes it easy and
natural for the Inbox page to get the right color too.
@chrisbobbe chrisbobbe force-pushed the pr-m3-and-recent-dms-page-fixes branch from 0ad5005 to 21dbae1 Compare November 16, 2023 04:16
@chrisbobbe
Copy link
Collaborator Author

Would you rebase this onto main?

Sure; done.

@gnprice gnprice merged commit 21dbae1 into zulip:main Nov 16, 2023
@gnprice
Copy link
Member

gnprice commented Nov 16, 2023

Thanks! Looks good; merging.

@chrisbobbe chrisbobbe deleted the pr-m3-and-recent-dms-page-fixes branch November 16, 2023 04:36
@chrisbobbe chrisbobbe mentioned this pull request Nov 20, 2023
Closed
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Nov 20, 2023
@chrisbobbe
compose: Shrink "send" button to match content-input height, like bef…
b47873f 
…ore M3

The way to get the smaller IconButton height we want has changed.
After migrating to Material Design 3, in zulip#380, we have to do this
different thing.

Fixes: zulip#398
@chrisbobbe chrisbobbe mentioned this pull request Nov 20, 2023
Merged
gnprice pushed a commit to chrisbobbe/zulip-flutter that referenced this pull request Nov 21, 2023
@chrisbobbe @gnprice
compose: Shrink "send" button to match content-input height, like bef…
c0a1064 
…ore M3

The way to get the smaller IconButton height we want has changed.
After migrating to Material Design 3, in zulip#380, we have to do this
different thing.

Fixes: zulip#398
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice Awaiting requested review from gnprice

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate to Material 3
2 participants
@chrisbobbe @gnprice