New design system colors #2309
New design system colors #2309
Conversation
| case is Colors.Text.Type: | ||
| namespace = "text/" | ||
| default: | ||
| namespace = "" |
There was a problem hiding this comment.
Based on this PR it looks like all the colors are defined in those three prefixes, which would suggest that we wouldn't expect the default case to be hit. Is that correct? If this should NOT be hit and we cant count on the compiler catching it, can we add an assertion failure here?
There was a problem hiding this comment.
That's a good point! However, I think we should keep the default case without an assertionFailure because we might want to define new color categories outside of a namespace.
There was a problem hiding this comment.
If we do that in the future, someone adding those colors would hit the assertion and they could remove it. In the current state of the code it looks like a mistake to add it here, so IMO it should get treated that way.
There was a problem hiding this comment.
That's a good point. I see your reasoning here, and it makes sense to catch unexpected cases early rather than silently defaulting to an empty namespace. I'll go ahead and add the assertionFailure to make it more explicit when a new color category is introduced outside of the expected namespaces.
| let colorName = "\(namespace)\(self.rawValue)" | ||
|
|
||
| // Retrieves the color from the asset catalog, defaulting to white if unavailable. | ||
| return UIColor(named: colorName) ?? .white |
There was a problem hiding this comment.
Similarly add an assertion failure here if we don't get that named color.
Generated by 🚫 Danger |
| } | ||
|
|
||
| assertionFailure("⚠️ Color not found in asset catalog: \(colorName)") | ||
| return .white |
There was a problem hiding this comment.
Nit: since most colors are used for text and UI elements, instead of backgrounds, it would be better if this was a color visible on the background. How about returning a default kickstarter green? But this is an edge case, I assume with the assert we'd never get here. 🤷♀️
There was a problem hiding this comment.
I was returning .white thinking of a neutral color, but I kind of agree — returning a green would likely be more visible in most cases. Which green are you thinking of, .ksr_create_700?
| case facebookDefault | ||
| case inverseDisabled | ||
| case inversePressed | ||
| case surfacePrimary |
There was a problem hiding this comment.
What does surface mean here? Is this a background?
There was a problem hiding this comment.
That's correct, surface refers to a background color, which is why it's under the Colors.Background enum. The idea is to stay consistent with the naming conventions used in Figma.
📲 What
Adaptive Colors system to have colors organized, and dynamic color retrieval from
.xcassets.background/,border/,text/) for clarity.🛠 How
AdaptativeColorsprotocol to handle color retrieval dynamically.Colorsstruct with namespaced enums (Background,Border,Text).adaptative()) to fetch colors dynamically based on enum type.👀 See
♿️ Accessibility
.xcassets.✅ Acceptance criteria
background/,border/,text/).⏰ TODO