Paths

Table of Contentst

-
native/themes/
-
themes/
-
colors.js

[native] introduce new shadesBlack90
ClosedPublic
Actions

Authored by ginsu on Nov 8 2023, 2:40 PM.

Details

Reviewers

atul
inka
rohan
ashoat

Commits

rCOMMfa42aa2dfeff: [native] introduce new shadesBlack90

Summary

This diff introduces a new shadesBlack90. The #191919 color is right now only used in one place so it never got a shade; however, the keyserver selection bottom sheet will use this + a lot of the web app redesign is going to use this color, so I think it makes sense to make this an official shade now.

Here is a screenshot to the new shades of black in the 2023 design system:

Screenshot 2023-11-08 at 6.22.00 PM.png (624×1 px, 51 KB)

Here is the figma doc: https://www.figma.com/file/fwKJ6eNgfxlx8a7WxSyAZL/2023-Design-System?type=design&node-id=1-426&mode=design

I still need to update the figma, but I want to do this after this diff gets accepted in case I need to make changes to this, but here is a task to track that
https://linear.app/comm/issue/DES-159/update-shades-of-black-in-figma

Test Plan

flow + went through the app to confirm that there were no regressions

This diff was handled using my code editor find + replace feature. Read through every value and all shades except for shadesBlack100 should be decremented by 10

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Nov 8 2023, 2:40 PM

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptNov 8 2023, 2:40 PM

ginsu edited the summary of this revision. (Show Details)Nov 8 2023, 2:55 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, inka, rohan.

ginsu attached a referenced file: F866271: Screenshot 2023-11-08 at 5.45.36 PM.png. (Show Details)

ginsu edited the summary of this revision. (Show Details)Nov 8 2023, 2:58 PM

ginsu added inline comments.

native/themes/colors.js
239 ↗	(On Diff #32980)	This is now the new `shadesBlack90`

ginsu added a child revision: D9790: [web] introduce new --shades-black-90.Nov 8 2023, 3:10 PM

ginsu edited the summary of this revision. (Show Details)Nov 8 2023, 3:23 PM

ginsu attached a referenced file: F866340: Screenshot 2023-11-08 at 6.22.00 PM.png. (Show Details)

ginsu published this revision for review.Nov 8 2023, 3:27 PM

Harbormaster completed remote builds in B23937: Diff 32980.Nov 8 2023, 5:16 PM

It seems like "90" and "80" are super close. Are you sure we need this new color? Can you explain / justify its introduction a bit more?
Is 50 / 60 / 70 / 80 / 90 actually arithmetically accurate? I have a suspicion it's not. If we actually took real mathematic percentages of pure black, what would it really be?

ashoat requested changes to this revision.Nov 9 2023, 3:44 AM

ashoat mentioned this in D9790: [web] introduce new --shades-black-90.

This revision now requires changes to proceed.Nov 9 2023, 3:45 AM

It seems like "90" and "80" are super close. Are you sure we need this new color? Can you explain / justify its introduction a bit more?

In the app there are a couple of places that use both "90" and "80":
The community drawer (90 is the darker color in the opened community and 80 is the drawer background)

Screenshot 2023-11-09 at 10.57.49 AM.png (1×1 px, 725 KB)

The keyserver selection modal (90 is the darker accent color around the keyserver details and 80 is the modal bottomsheet backgroud)

Screenshot 2023-11-09 at 11.01.39 AM.png (1×1 px, 737 KB)

We will also have this new 90 color in a lot of the web app redesign (the red is just to show where this new color value would be placed):

Screenshot 2023-11-09 at 11.12.43 AM.png (1×1 px, 345 KB)

Is 50 / 60 / 70 / 80 / 90 actually arithmetically accurate? I have a suspicion it's not. If we actually took real mathematic percentages of pure black, what would it really be?

I am not too sure about this. I just took the color value that was already in Figma. Picking the exact arithmetically accurate color value is a bit outside my design expertise, and I wasn't sure if I should spend a lot of time on this, but can do so if we think it's important.

If we still feel strongly about not wanting to introduce this color, the best way forward will probably be to migrate the new 90 value to use the current black 100; however, things will look like this (which I don't think visually look great):

Screenshot 2023-11-09 at 11.16.23 AM.png (1×1 px, 737 KB)

Screenshot 2023-11-09 at 11.16.17 AM.png (1×1 px, 729 KB)

ginsu attached a referenced file: F867377: Screenshot 2023-11-09 at 11.16.17 AM.png. (Show Details)Nov 9 2023, 8:19 AM

ginsu attached a referenced file: F867378: Screenshot 2023-11-09 at 11.16.23 AM.png. (Show Details)

ginsu attached a referenced file: F867373: Screenshot 2023-11-09 at 11.12.43 AM.png. (Show Details)

ginsu attached a referenced file: F867337: Screenshot 2023-11-09 at 11.01.39 AM.png. (Show Details)

ginsu attached a referenced file: F867334: Screenshot 2023-11-09 at 10.57.49 AM.png. (Show Details)

Thanks for explaining. Let's keep the new color, but I'd like an answer to my question about the arithmetic. You're making an opinionated change here by changing our existing colors (eg. 80 -> 70) – that change should be an informed one.

Picking the exact arithmetically accurate color value is a bit outside my design expertise

It really should not be so difficult for you to calculate. If you're confused about it, feel free to ask @atul. This should take you like 5min.

Is 50 / 60 / 70 / 80 / 90 actually arithmetically accurate? I have a suspicion it's not. If we actually took real mathematic percentages of pure black, what would it really be?

As it stands right now the mathematic percentages of pure black for our shadesBlack variables are:

shadesBlack100 (#0a0a0a): ~96%
shadesBlack90 (#191919): ~90%
shadesBlack80 (#1f1f1f): ~88%
shadesBlack70 (#404040): ~75%
shadesBlack60 (#666666): 60%
shadesBlack50 (#808080): ~50%

Screenshot 2023-11-09 at 5.15.29 PM.png (558×320 px, 55 KB)

Based on this, the only color variables that I updated that are not arithmetically accurate are shadesBlack80 and shadesBlack70 (I did not touch shadesBlack100 in this diff). I think the best way to move forward is that we should update shadesBlack80 => shadesBlack85 and shadesBlack70 => shadesBlack75. (If we do this it might also be good to update shadesBlack100 => shadesBlack95).

Another option is that we could also update the hex value to be closer to the names, but I think this could be pretty risky and could introduce a bunch of regressions with our colors in the app if we are not super careful

ginsu attached a referenced file: F867980: Screenshot 2023-11-09 at 5.15.29 PM.png. (Show Details)Nov 9 2023, 2:34 PM

Oh whoa, so the original change actually makes shadesBlack60 and some others more accurate.

I think the best way to move forward is that we should update shadesBlack80 => shadesBlack85 and shadesBlack70 => shadesBlack75. (If we do this it might also be good to update shadesBlack100 => shadesBlack95).

Makes sense to me! Let's do it.

update

ginsu planned changes to this revision.Nov 9 2023, 3:39 PM

forgot to hit save on my editor

I did some spot-checking but didn't review each line

This revision is now accepted and ready to land.Nov 9 2023, 3:43 PM

Harbormaster completed remote builds in B23982: Diff 33040.Nov 9 2023, 3:58 PM

Harbormaster completed remote builds in B23983: Diff 33041.Nov 9 2023, 4:18 PM

Closed by commit rCOMMfa42aa2dfeff: [native] introduce new shadesBlack90 (authored by ginsu). · Explain WhyNov 9 2023, 10:19 PM

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMMfa42aa2dfeff: [native] introduce new shadesBlack90.

Revision Contents
Changeset List

Path

Size

native/

themes/

colors.js

147 lines

Diff 33045

View Options

native/themes/colors.js

	import ExpoModulesCore			import ExpoModulesCore
				import CryptoKit

	public class AESCryptoModule: Module {			public class AESCryptoModule: Module {
	public func definition() -> ModuleDefinition {			public func definition() -> ModuleDefinition {
	Name("AESCrypto")			Name("AESCrypto")

	Function("hello") { () -> String in			Function("generateKey", generateKey)
	return "Hello world! 👋"
	}			}
	}			}

				// MARK: - Function implementations

				private func generateKey(destination: Uint8Array) throws {
				let key = SymmetricKey(size: .bits256)
				guard destination.byteLength * 8 == key.bitCount else {
				throw InvalidKeyLengthException()
				}
				atulUnsubmitted Not Done Inline Actions Before we even generate `key` via `SymmetricKey`, should we check that `destination.byteLength` is `32` and throw `InvalidDestinationSizeException()` if not? It seems like `destination` is the input that we want to validate, so we should just do that directly? It's not really so much that `SymmetricKey` is giving us incorrect size `key` since it's (I'm assuming) always going to give us exactly the size we're asking for? Feel free to re-request review if there's something I'm missing here. atul: Before we even generate `key` via `SymmetricKey`, should we check that `destination.byteLength`…
				bartekAuthorUnsubmitted Done Inline Actions The only reason the guard is below `SymmetricKey` is that I wanted to use its `bitCount` property to get the desired destination length, but you're right that it can be done this way: guard destination.byteLength == KEY_SIZE else { throw InvalidKeyLengthException() } let key = SymmetricKey(size: .bits256) I'll refactor this as it is more clear solution bartek: The only reason the guard is below `SymmetricKey` is that I wanted to use its `bitCount`…

				key.withUnsafeBytes { bytes in
				let _ = bytes.copyBytes(to: destination.rawBufferPtr())
				}
				}

				// MARK: - Utilities

				extension TypedArray {
				func rawBufferPtr() -> UnsafeMutableRawBufferPointer {
				UnsafeMutableRawBufferPointer(start: self.rawPointer,
				count: self.byteLength)
				}
				}

				// MARK: - Exception definitions

				private class InvalidKeyLengthException: Exception {
				override var reason: String {
				"The AES key has invalid length"
				}
	}			}

[native] introduce new shadesBlack90ClosedPublicActions