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.

Sign up for GitHub

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

Jump to bottom

Set up iOS alias color tokens #2481

Merged
25 commits merged into from
Jan 10, 2023
Merged

Set up iOS alias color tokens #2481

25 commits merged into from
Jan 10, 2023

Conversation

lyzhan7
Copy link
Contributor

@lyzhan7 lyzhan7 commented Jan 4, 2023 •
edited
Loading

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

Description of changes

Set up iOS alias color tokens in FURN (just neutral and brand colors for now; shared colors will be done in a future PR), so the tokens specified in updated Fluent2 designs can be accessed through the theme.

  1. The pipeline output is retrieved by getTokens.ios.ts
    • Also moved apple-theme/getiOSTokens.ts to theme-tokens/getTokens.ios.ts, as per Refactoring - Make theme tokens flow consistent #2190. Slightly unrelated but also recombined the existing android/windows getTokens/getShadowTokens files - they were separated to make android token integration more simple but since that work is complete, should be fine to recombine them and have less files
  2. The output gets mapped in an AliasColorTokens object through mapPipelineToTheme
    • The AliasColorTokens object was updated to include some new mobile tokens
    • The existing mapPipelineToTheme method in the mapPipelineToTheme.ios.ts file was copied from the original mapPipelineToTheme.ts file. Updated it to match designs - for this file reviewing commit by commit should make the changes more clear
  3. The AliasColorTokens object gets added to the iOS color palette in paletteFromAppleColors by createiOSColorAliasTokens

Out of scope for this PR:

  1. Shared colors (ex. dangerbackground1) - there's been some questions raised about the definitions for these tokens so they haven't been added to the design-tokens package yet
  2. Moving all createAliasTokens files from platform-specific themes into the theme-tokens folder - this seemed to have been planned for all the platforms but none of them do this yet. Planning to do this in a future PR

Verification

Initial verification since the actual tokens aren't part of FURN yet - copied some tokens into the IOS alias-tokens files in node_modules, and checked that they were part of the final iOS palette.

No visual changes otherwise. Will add these tokens to the Notification component in a future PR for additional verification.

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

@lyzhan7 lyzhan7 mentioned this pull request Jan 4, 2023
Merged
10 tasks
@lyzhan7 lyzhan7 marked this pull request as ready for review January 5, 2023 16:19
@lyzhan7 lyzhan7 requested a review from a team as a code owner January 5, 2023 16:19
ghost pushed a commit that referenced this pull request Jan 6, 2023
@lyzhan7
Change default theme from a const object to a function that returns t…
984ee82 
…he object (#2483)

### Platforms Impacted
- [x] iOS
- [x] macOS
- [x] win32 (Office)
- [x] windows
- [x] android

### Description of changes

Update the default theme fluent object to be a method that returns an object. 

This issue is blocking #2481:
- the updated designs for iOS tokens include some new tokens that aren't in the default theme
- mapPipelineToTheme.ios.ts was updated to reflect these designs
- however, the default theme is still getting created and is trying to running the iOS code for mapPipelineToTheme, which causes failures because the iOS alias token set now includes tokens that aren't in the default theme (win32) token set
- changing the default theme from a const object to a method prevents the default theme from getting created automatically

### Verification

No visual changes
Check if CI passes

### Pull request checklist

This PR has considered (when applicable):
- [ ] Automated Tests
- [ ] Documentation and examples
- [ ] Keyboard Accessibility
- [ ] Voiceover
- [ ] Internationalization and Right-to-left Layouts
This was referenced Jan 9, 2023
Merged
Merged
@lyzhan7
Copy link
Contributor Author

lyzhan7 commented Jan 10, 2023 •
edited
Loading

Currently waiting on #2502 to go in first - that should be the last blocker for the PR checks to pass (was just merged)

@lyzhan7 lyzhan7 added the AutoMerge 🔁 Automatically merge when PR requirements met label Jan 10, 2023
@ghost
Copy link

ghost commented Jan 10, 2023

Hello @lyzhan7!

Because this pull request has the AutoMerge :repeat: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit bd23a4a into microsoft:main Jan 10, 2023
@lyzhan7 lyzhan7 deleted the iOS-alias-color-tokens branch January 10, 2023 19:09
@lyzhan7 lyzhan7 mentioned this pull request Jan 13, 2023
Merged
10 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankraj12 ankraj12 ankraj12 approved these changes

@Saadnajmi Saadnajmi Saadnajmi left review comments

@harrieshin harrieshin harrieshin approved these changes

Assignees
No one assigned
Labels
AutoMerge 🔁 Automatically merge when PR requirements met
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lyzhan7 @Saadnajmi @harrieshin @ankraj12