Skip to content
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

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

alpavanoglu
Copy link
Contributor

@alpavanoglu alpavanoglu commented Jan 16, 2024

Refs #p1702909565644119/1702665510.828399-slack-C04TXQU42HM
Figma File: LyzJJa6ANKToUswWmRrZee-fi-102_186

Testing Steps:

  1. Install and login Jetpack
  2. Navigate to "Design System" and check if Secondary, Tertiary and Quaternary background color values match the designs on both themes.
  3. Make sure "Divider" color is removed from Color.xcassets in Design System Module

Regression Notes

  1. Potential unintended areas of impact
    The callsites for the changed values.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manually tested them.

  3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@dangermattic
Copy link
Collaborator

dangermattic commented Jan 16, 2024

1 Warning
⚠️ View files have been modified, but no screenshot is included in the pull request. Consider adding some for clarity.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 16, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr22402-1afcf93
Version24.0
Bundle IDorg.wordpress.alpha
Commit1afcf93
App Center BuildWPiOS - One-Offs #8652
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 16, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr22402-1afcf93
Version24.0
Bundle IDcom.jetpack.alpha
Commit1afcf93
App Center Buildjetpack-installable-builds #7680
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@mokagio
Copy link
Contributor

mokagio commented Jan 22, 2024

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.

@mokagio mokagio modified the milestones: 24.1, 24.2 Jan 22, 2024
@@ -35,8 +35,6 @@ public extension Color {
private static let wordPress = colorWithModuleBundle(colorName: DesignSystemColorNames.Background.wordPress)
}

public static let divider = colorWithModuleBundle(colorName: DesignSystemColorNames.divider)
Copy link
Contributor

@salimbraksa salimbraksa Feb 1, 2024

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
}

Copy link
Contributor Author

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?

@salimbraksa
Copy link
Contributor

salimbraksa commented Feb 1, 2024

Hey @alpavanoglu -- the hex value of background tertiary is just #C2. Maybe that's a typo?

@alpavanoglu
Copy link
Contributor Author

alpavanoglu commented Feb 2, 2024

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.

@salimbraksa
Copy link
Contributor

I feel like this has happened before. I think when it is a repeating hex value like #C2C2C2

Yes I guess so, not sure if Xcode supports that behavior. However, on Figma, the hex value is #C2C2C6

@peril-wordpress-mobile
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@mokagio
Copy link
Contributor

mokagio commented Feb 4, 2024

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.

@mokagio mokagio modified the milestones: 24.2, 24.3 Feb 4, 2024
@alpavanoglu alpavanoglu enabled auto-merge February 5, 2024 11:39
Copy link
Contributor

@salimbraksa salimbraksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

@alpavanoglu alpavanoglu merged commit c52d38c into trunk Feb 5, 2024
23 of 24 checks passed
@alpavanoglu alpavanoglu deleted the design-system-color-update branch February 5, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants