-
Notifications
You must be signed in to change notification settings - Fork 255
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
widgets: Pull out reusable UnreadCountBadge widget; grow support for stream colors #371
Changes from all commits
d9eb0d3
a73175f
d990df5
0e33d57
255fff3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
import 'dart:ui'; | ||
|
||
import 'package:flutter_color_models/flutter_color_models.dart'; | ||
import 'package:flutter/material.dart'; | ||
|
||
import 'text.dart'; | ||
|
||
/// A widget to display a given number of unreads in a conversation. | ||
/// | ||
/// Implements the design for these in Figma: | ||
/// <https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=341%3A12387&mode=dev> | ||
class UnreadCountBadge extends StatelessWidget { | ||
const UnreadCountBadge({ | ||
super.key, | ||
required this.count, | ||
required this.baseStreamColor, | ||
this.bold = false, | ||
}); | ||
|
||
final int count; | ||
final bool bold; | ||
|
||
/// A base stream color, from a stream subscription in user data, or null. | ||
/// | ||
/// If not null, the background will be colored with an appropriate | ||
/// transformation of this. | ||
/// | ||
/// If null, the default neutral background will be used. | ||
final Color? baseStreamColor; | ||
|
||
@visibleForTesting | ||
Color getBackgroundColor() { | ||
if (baseStreamColor == null) { | ||
return const Color.fromRGBO(102, 102, 153, 0.15); | ||
} | ||
|
||
// Follows `.unread-count` in Vlad's replit: | ||
// <https://replit.com/@VladKorobov/zulip-sidebar#script.js> | ||
// <https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20.23F117.20.22Inbox.22.20screen/near/1624484> | ||
|
||
// The design uses "LCH", not "LAB", but we haven't found a Dart libary | ||
// that can work with LCH: | ||
// <https://replit.com/@VladKorobov/zulip-sidebar#script.js> | ||
// <https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20.23F117.20.22Inbox.22.20screen/near/1677537> | ||
// We use LAB because some quick reading suggests that the "L" axis | ||
// is the same in both representations: | ||
// <https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/lch> | ||
// and because the design doesn't use the LCH representation except to | ||
// adjust an "L" value. | ||
// | ||
// TODO try LCH; see linked discussion | ||
// TODO fix bug where our results differ from the replit's (see unit tests) | ||
// TODO profiling for expensive computation | ||
final asLab = LabColor.fromColor(baseStreamColor!); | ||
return asLab | ||
.copyWith(lightness: asLab.lightness.clamp(30, 70)) | ||
.toColor() | ||
.withOpacity(0.3); | ||
} | ||
|
||
@override | ||
Widget build(BuildContext context) { | ||
return DecoratedBox( | ||
decoration: BoxDecoration( | ||
borderRadius: BorderRadius.circular(3), | ||
color: getBackgroundColor(), | ||
), | ||
child: Padding( | ||
padding: const EdgeInsets.fromLTRB(4, 0, 4, 1), | ||
child: Text( | ||
style: const TextStyle( | ||
fontFamily: 'Source Sans 3', | ||
fontSize: 16, | ||
height: (18 / 16), | ||
fontFeatures: [FontFeature.enable('smcp')], // small caps | ||
color: Color(0xFF222222), | ||
).merge(bold | ||
? weightVariableTextStyle(context, wght: 600, wghtIfPlatformRequestsBold: 900) | ||
: weightVariableTextStyle(context)), | ||
count.toString()))); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import 'dart:ui'; | ||
|
||
import 'package:checks/checks.dart'; | ||
import 'package:zulip/widgets/unread_count_badge.dart'; | ||
|
||
extension UnreadCountBadgeChecks on Subject<UnreadCountBadge> { | ||
Subject<int> get count => has((b) => b.count, 'count'); | ||
Subject<bool> get bold => has((b) => b.bold, 'bold'); | ||
Subject<Color> get backgroundColor => has((b) => b.getBackgroundColor(), 'background color'); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
import 'package:checks/checks.dart'; | ||
import 'package:flutter/widgets.dart'; | ||
|
||
import 'package:flutter_test/flutter_test.dart'; | ||
import 'package:zulip/widgets/unread_count_badge.dart'; | ||
|
||
import 'unread_count_badge_checks.dart'; | ||
|
||
void main() { | ||
group('UnreadCountBadge', () { | ||
testWidgets('smoke test; no crash', (tester) async { | ||
await tester.pumpWidget( | ||
const Directionality(textDirection: TextDirection.ltr, | ||
child: UnreadCountBadge(count: 1, baseStreamColor: null))); | ||
tester.widget(find.text("1")); | ||
}); | ||
|
||
test('colors', () { | ||
void runCheck(Color? baseStreamColor, Color expectedBackgroundColor) { | ||
check(UnreadCountBadge(count: 1, baseStreamColor: baseStreamColor)) | ||
.backgroundColor.equals(expectedBackgroundColor); | ||
} | ||
|
||
runCheck(null, const Color(0x26666699)); | ||
|
||
// Check against everything in ZULIP_ASSIGNMENT_COLORS and EXTREME_COLORS | ||
// in <https://replit.com/@VladKorobov/zulip-sidebar#script.js>. | ||
// On how to extract expected results from the replit, see: | ||
// https://github.com/zulip/zulip-flutter/pull/371#discussion_r1393643523 | ||
|
||
// TODO Fix bug causing our implementation's results to differ from the | ||
// replit's. Where they differ, see comment with what the replit gives. | ||
|
||
// ZULIP_ASSIGNMENT_COLORS | ||
runCheck(const Color(0xff76ce90), const Color(0x4d65bd80)); | ||
runCheck(const Color(0xfffae589), const Color(0x4dbdab53)); // 0x4dbdaa52 | ||
runCheck(const Color(0xffa6c7e5), const Color(0x4d8eafcc)); // 0x4d8fb0cd | ||
runCheck(const Color(0xffe79ab5), const Color(0x4de295b0)); // 0x4de194af | ||
runCheck(const Color(0xffbfd56f), const Color(0x4d9eb551)); // 0x4d9eb450 | ||
runCheck(const Color(0xfff4ae55), const Color(0x4de19d45)); // 0x4de09c44 | ||
runCheck(const Color(0xffb0a5fd), const Color(0x4daba0f8)); // 0x4daca2f9 | ||
runCheck(const Color(0xffaddfe5), const Color(0x4d83b4b9)); // 0x4d83b4ba | ||
runCheck(const Color(0xfff5ce6e), const Color(0x4dcba749)); // 0x4dcaa648 | ||
runCheck(const Color(0xffc2726a), const Color(0x4dc2726a)); | ||
runCheck(const Color(0xff94c849), const Color(0x4d86ba3c)); // 0x4d86ba3b | ||
runCheck(const Color(0xffbd86e5), const Color(0x4dbd86e5)); | ||
runCheck(const Color(0xffee7e4a), const Color(0x4dee7e4a)); | ||
runCheck(const Color(0xffa6dcbf), const Color(0x4d82b69b)); // 0x4d82b79b | ||
runCheck(const Color(0xff95a5fd), const Color(0x4d95a5fd)); | ||
runCheck(const Color(0xff53a063), const Color(0x4d53a063)); | ||
runCheck(const Color(0xff9987e1), const Color(0x4d9987e1)); | ||
runCheck(const Color(0xffe4523d), const Color(0x4de4523d)); | ||
runCheck(const Color(0xffc2c2c2), const Color(0x4dababab)); | ||
runCheck(const Color(0xff4f8de4), const Color(0x4d4f8de4)); | ||
runCheck(const Color(0xffc6a8ad), const Color(0x4dc2a4a9)); // 0x4dc1a4a9 | ||
runCheck(const Color(0xffe7cc4d), const Color(0x4dc3ab2a)); // 0x4dc2aa28 | ||
runCheck(const Color(0xffc8bebf), const Color(0x4db3a9aa)); | ||
runCheck(const Color(0xffa47462), const Color(0x4da47462)); | ||
|
||
// EXTREME_COLORS | ||
runCheck(const Color(0xFFFFFFFF), const Color(0x4dababab)); | ||
runCheck(const Color(0xFF000000), const Color(0x4d474747)); | ||
runCheck(const Color(0xFFD3D3D3), const Color(0x4dababab)); | ||
runCheck(const Color(0xFFA9A9A9), const Color(0x4da9a9a9)); | ||
runCheck(const Color(0xFF808080), const Color(0x4d808080)); | ||
runCheck(const Color(0xFFFFFF00), const Color(0x4dacb300)); // 0x4dacb200 | ||
runCheck(const Color(0xFFFF0000), const Color(0x4dff0000)); | ||
runCheck(const Color(0xFF008000), const Color(0x4d008000)); | ||
runCheck(const Color(0xFF0000FF), const Color(0x4d0000ff)); // 0x4d0902ff | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this difference of 0x00 vs 0x09 might be the biggest in any channel in any of these test cases, and all the others I spotted were a difference of 1 or 2. Is that right? If it's almost always 1 or 2, that seems pretty mild, though it'd still ultimately be good to track down and resolve. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I skimmed through them just now, looking for any difference bigger than 0x09. I didn't find one. Then I realized you also asked if all the rest just had differences of 1 or 2, and I wasn't keeping track of those…I think there were a few that had a difference of 3. Here's one with a difference of 3 and of 5: runCheck(const Color(0xFF000080), const Color(0x4d492bae)); // 0x4d4b2eb3 |
||
runCheck(const Color(0xFFEE82EE), const Color(0x4dee82ee)); | ||
runCheck(const Color(0xFFFFA500), const Color(0x4def9800)); // 0x4ded9600 | ||
runCheck(const Color(0xFF800080), const Color(0x4d810181)); // 0x4d810281 | ||
runCheck(const Color(0xFF00FFFF), const Color(0x4d00c2c3)); // 0x4d00c3c5 | ||
runCheck(const Color(0xFFFF00FF), const Color(0x4dff00ff)); | ||
runCheck(const Color(0xFF00FF00), const Color(0x4d00cb00)); | ||
runCheck(const Color(0xFF800000), const Color(0x4d8d140c)); // 0x4d8b130b | ||
runCheck(const Color(0xFF008080), const Color(0x4d008080)); | ||
runCheck(const Color(0xFF000080), const Color(0x4d492bae)); // 0x4d4b2eb3 | ||
runCheck(const Color(0xFFFFFFE0), const Color(0x4dadad90)); // 0x4dacad90 | ||
runCheck(const Color(0xFFFF69B4), const Color(0x4dff69b4)); | ||
}); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. How did you extract the replit's results?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forked the replit (link to the replit) and added some code to run all the colors through
getCounterBackgroundColor
(since that's what gives the background color for the counters in the replit) and print the results to the console.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Is it easy to link to that forked replit?
That just seems like it'll be helpful whenever someone (be it you a few months in the future, or someone else) goes to try to resolve this TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I deleted the fork. I couldn't easily figure out version control, and I didn't want to let my future self mix up which code was Vlad's vs. mine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I added a link to your previous reply, just in the interest of making the information we do have handy now remain handy when someone takes up this TODO.