-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Design System] Update color values to match Figma #22402
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
I'm going to bump this to the next release because we'll be code freezing 24.1 today and this hasn't been reviewed yet. If these changes cannot wait two weeks and it's important that it makes it into this release, let me know and we'll organize a new beta once ready. |
@@ -35,8 +35,6 @@ public extension Color { | |||
private static let wordPress = colorWithModuleBundle(colorName: DesignSystemColorNames.Background.wordPress) | |||
} | |||
|
|||
public static let divider = colorWithModuleBundle(colorName: DesignSystemColorNames.divider) |
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.
nit: it would be great to keep this property for convenience, similar to UIColor.separator
.
public static var divider: Color {
return Background.tertiary
}
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.
Hmm. Do you mean to keep this but use the background tertiary as its value? I am not sure about it. Reason being is that on Figma design will mention Background.tertiary
and most likely the developer will use that value. If we use another naming even for convenience, that could break the sync between platforms. After all the whole point is that the names on Android, iOS and Figma to fully match. Wdyt?
Hey @alpavanoglu -- the hex value of background tertiary is just #C2. Maybe that's a typo? |
I feel like this has happened before. I think when it is a repeating hex value like #C2C2C2, it shortens the value automatically. I'll look into it again. Idk if it intentional by Xcode or not. Maybe I mis-remember it. In any case the value should show on the gallery. |
Yes I guess so, not sure if Xcode supports that behavior. However, on Figma, the hex value is #C2C2C6 |
Generated by 🚫 dangerJS |
I'm going to bump this to the next release because we'll be code freezing 24.2 today and this hasn't been reviewed yet. If these changes cannot wait two weeks and it's important that it makes it into this release, let me know and we'll organize a new beta once ready. |
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.
LGTM! 🚀
Refs #p1702909565644119/1702665510.828399-slack-C04TXQU42HM
Figma File: LyzJJa6ANKToUswWmRrZee-fi-102_186
Testing Steps:
Regression Notes
Potential unintended areas of impact
The callsites for the changed values.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manually tested them.
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: