Skip to content
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.

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

compose: Make compose-box border more prominent in dark mode #1233

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

lakshya1goel
Copy link
Contributor

@lakshya1goel lakshya1goel commented Dec 29, 2024

This PR will update the top border and add shadow effect to the top of the compose box to make it more visible in dark mode

For reference, see the Figma design:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5908-64038&t=alSmAmdRXFDwT4IT-1

Fixes: #1207

Before After
WhatsApp Image 2024-12-29 at 2 28 00 PM WhatsApp Image 2024-12-29 at 2 28 01 PM

@lakshya1goel lakshya1goel marked this pull request as draft December 29, 2024 09:11
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I see this is a draft, but I gave it a quick skim and I noticed a different direction I'd like to take. 🙂

Since the shadow is only meant to be applied in dark theme, I'd like to avoid asking Flutter to do any computations for it when in light mode. For that, let's add a ComposeBoxTheme class in lib/widgets/compose_box.dart, with just one field boxShadow that's null in light theme and non-null in dark theme. ContentTheme and MessageListTheme are examples of the pattern I have in mind. So, something like this:

Proposal
/// Compose-box styles that differ between light and dark theme.
///
/// These styles will animate on theme changes (with help from [lerp]).
class ComposeBoxTheme extends ThemeExtension<ComposeBoxTheme> {
  factory ComposeBoxTheme.light(BuildContext context) {
    return ComposeBoxTheme._(
      boxShadow: null,
    );
  }

  factory ComposeBoxTheme.dark(BuildContext context) {
    return ComposeBoxTheme._(
      boxShadow: [BoxShadow(
        color: DesignVariables.dark.bgTopBar,
        offset: const Offset(0, -4),
        blurRadius: 16,
        spreadRadius: 0,
      )],
    );
  }

  ComposeBoxTheme._({
    required this.boxShadow,
  });

  /// The [ComposeBoxTheme] from the context's active theme.
  ///
  /// The [ThemeData] must include [ComposeBoxTheme] in [ThemeData.extensions].
  static ComposeBoxTheme of(BuildContext context) {
    final theme = Theme.of(context);
    final extension = theme.extension<ComposeBoxTheme>();
    assert(extension != null);
    return extension!;
  }

  final List<BoxShadow>? boxShadow;

  @override
  ComposeBoxTheme copyWith({
    List<BoxShadow>? boxShadow,
  }) {
    return ComposeBoxTheme._(
      boxShadow: boxShadow ?? this.boxShadow,
    );
  }

  @override
  ComposeBoxTheme lerp(ComposeBoxTheme other, double t) {
    if (identical(this, other)) {
      return this;
    }
    return ComposeBoxTheme._(
      boxShadow: BoxShadow.lerpList(boxShadow, other.boxShadow, t)!,
    );
  }
}

Then the caller can say boxShadow: ComposeBoxTheme.of(context).boxShadow.

Note the use of DesignVariables.dark.bgTopBar instead of adding a new design variable. The Figma doesn't add any new design variables. Also, it's DesignVariables.dark.bgTopBar instead of DesignVariables.dark().bgTopBar because of a small optimization, #1236; I suggest rebasing your work atop that PR to get that optimization.

@lakshya1goel
Copy link
Contributor Author

Thanks for the review, I am working on it and will update it as required ASAP.
Thanks!

@lakshya1goel lakshya1goel marked this pull request as ready for review January 4, 2025 06:50
@PIG208 PIG208 mentioned this pull request Jan 9, 2025
@PIG208 PIG208 self-assigned this Jan 15, 2025
@PIG208 PIG208 added the maintainer review PR ready for review by Zulip maintainers label Jan 15, 2025
@PIG208
Copy link
Member

PIG208 commented Jan 15, 2025

There is a merge conflict. Could you resolve it? Thanks!

@lakshya1goel lakshya1goel force-pushed the issue1207 branch 4 times, most recently from c6bb562 to 260457a Compare January 17, 2025 13:28
Copy link

@0pni 0pni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

@lakshya1goel
Copy link
Contributor Author

lakshya1goel commented Jan 18, 2025

Hi, @PIG208 I have resolved the conflict and pushed the changes atop #1290 for the sake of CI.
Please have a look on the changes and let me know if anything else is required.
Thanks!

@lakshya1goel
Copy link
Contributor Author

Hello, @chrisbobbe thanks for mentioning the changes. I have updated it accordingly.
Please have a look and let me know if anything else is required.
Thanks!

Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Left some comments, where only the commit message nit is actionable right now.

@lakshya1goel
Copy link
Contributor Author

Thank you for the reviews @PIG208 and @chrisbobbe , I have made the required changes. Please let me know if anything else is required.

@PIG208 PIG208 added the integration review Added by maintainers when PR may be ready for integration label Jan 27, 2025
@PIG208
Copy link
Member

PIG208 commented Jan 27, 2025

LGTM with one nit! Adding the integration tag but leaving the maintainer review in case Chris has anything to add.

@lakshya1goel
Copy link
Contributor Author

Thanks for the review @PIG208, pushed the revision.

Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Very small comments below.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lakshya1goel for taking care of this, and @chrisbobbe and @PIG208 for the previous reviews!

These changes all look good. Just a couple of comments — one on the commit message, below, and two on the PR text:

The PR description is backward from what the PR does:

to ensure it is subtle in dark mode.

I think this is the same issue as #1233 (comment) .

The PR title says:

compose: Compose-box border is subtle in dark mode

which matches the issue title. But a PR title should say what the PR does, not describe the status quo from before the PR.

A good solution for both would be to edit the PR title and description to match the commit message. In general a PR title and description do a very similar job to a commit message; so when a PR has just one commit, they should usually be very similar to that one commit's message.

@lakshya1goel lakshya1goel changed the title compose: Compose-box border is subtle in dark mode compose: Make compose-box border more prominent in dark mode Feb 1, 2025
@lakshya1goel lakshya1goel force-pushed the issue1207 branch 2 times, most recently from d354591 to 2a0d1c9 Compare February 1, 2025 06:50
@lakshya1goel
Copy link
Contributor Author

Thanks for the review @gnprice, updated the PR accordingly, PTAL.

@lakshya1goel lakshya1goel requested a review from gnprice February 1, 2025 06:54
Updated the top border and added shadow effect to the top of
the compose box to make it more visible in dark mode.

For reference, see the Figma design:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5908-64038&t=alSmAmdRXFDwT4IT-1

Fixes: zulip#1207
@gnprice gnprice merged commit 7659479 into zulip:main Feb 4, 2025
1 check failed
@gnprice
Copy link
Member

gnprice commented Feb 4, 2025

Thanks! Looks good; merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compose-box border is subtle in dark mode
5 participants