-
Notifications
You must be signed in to change notification settings - Fork 308
model: Implement dark-mode stream color swatches #643
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
Conversation
Here is how I generated the "expected" values to put in the tests. (This comment is adapted from #381 (comment) ; this time for dark mode.) Here's the code I added in the replit, just under const dartColorIntFromHexString = (hexString) => {
const lowercaseWithoutHash = hexString.substring(1).toLowerCase();
switch (lowercaseWithoutHash.length) {
case 6: // opaque (alpha omitted)
return `0xff${lowercaseWithoutHash}`;
case 8: // not opaque
// convert rrggbbaa to aarrggbb
return '0x'
+ lowercaseWithoutHash.substring(6, 8)
+ lowercaseWithoutHash.substring(0, 2);
default:
throw new Error();
}
};
const dartCheckLine = (baseHex, expectedHex) => {
return `runCheck(${dartColorIntFromHexString(baseHex)}, const Color(${dartColorIntFromHexString(expectedHex)}));\n`;
};
let iconOnPlainBackgroundResult = '';
let iconOnBarBackgroundResult = '';
let barBackgroundResult = '';
ZULIP_ASSIGNMENT_COLORS.forEach(color => {
const correctedColor = correctStreamColor({ color });
const barBackground = getRecipientBarColor({ color: correctStreamColor({ color }), darkMode: true });
const iconOnPlainBackground = correctedColor;
const iconOnBarBackground = correctedColor; // follows web, not replit
iconOnPlainBackgroundResult += dartCheckLine(color, iconOnPlainBackground);
iconOnBarBackgroundResult += dartCheckLine(color, iconOnBarBackground);
barBackgroundResult += dartCheckLine(color, barBackground);
});
console.log(iconOnPlainBackgroundResult);
console.log(iconOnBarBackgroundResult);
console.log(barBackgroundResult); |
d12d75e
to
1f2c649
Compare
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.
Thanks! Just small comments below. (Modulo the review I just left on #642.)
lib/api/model/model.dart
Outdated
// TODO fix bug where our results differ from the replit's (see unit tests) | ||
StreamColor.iconOnBarBackground: | ||
clamped20to75AsHsl | ||
.withLightness((clamped20to75AsHsl.lightness - 0.12).clamp(0.0, 1.0)) |
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.
use Flutter's clampDouble
instead; see its docs
lib/api/model/model.dart
Outdated
// Follows `.recepient` in Vlad's replit: | ||
// <https://replit.com/@VladKorobov/zulip-topic-feed-colors#script.js> | ||
// | ||
// TODO I think [LabColor.interpolate] doesn't actually do LAB mixing; | ||
// it just calls up to the superclass method [ColorModel.interpolate]: | ||
// <https://pub.dev/documentation/flutter_color_models/latest/flutter_color_models/ColorModel/interpolate.html> | ||
// which does ordinary RGB mixing. Investigate and send a PR? | ||
// TODO fix bug where our results differ from the replit's (see unit tests) | ||
StreamColor.barBackground: |
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.
This comment is all the same as in the light case, right?
Let's try to deduplicate comments that don't differ. It makes it easier to find the spots where there is new information in these comments (after reading the other version's comments), like for iconOnBarBackground
.
1f2c649
to
3212c57
Compare
Thanks for the review! Revision pushed, and now this is atop instead of . |
…essary We'll get an error if `clamped20to75AsHsl.lightness - 0.12` ends up being below 0.0 or above 1.0... ...which probably can't happen? The value `clamped20to75AsHsl.lightness` is the "L" of an HSL representation of a certain color. That certain color is *supposed* to be the result of clamping an *LCH* "L" value between 20 and 75...but it actually comes from clamping a LAB color's "L" value between those numbers...but we have a comment saying the "L" axis might be the same between LCH and LAB... ...but I think HSL "L" probably doesn't match either LCH "L" or LAB "L"...which still might not matter in this case? Anyway, with some uncertainty, it seems like it might be wise to do this clamping to avoid an error.
Instead of a hard-coded value.
Thanks for the revision! LGTM — merging. |
3212c57
to
c64d7a0
Compare
Stacked atop
#642#654.We added the light-mode color computations in #381.
We're not ready to use these yet, but the color computations are
unlikely to change before that time comes, and finding the right
ones is a chore that's good to get done.
Related: #95