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

Memoize theme creation for iOS, macOS, android #2485

Merged
merged 4 commits into from
Jan 26, 2023

Conversation

lyzhan7
Copy link
Contributor

@lyzhan7 lyzhan7 commented Jan 4, 2023

Platforms Impacted

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

Description of changes

There was a suggestion in PR #2483 to memoize the method creating the default theme to prevent the method from creating a new theme object every time it's called. This PR is just making the same change for the iOS, macOS, and android themes.

Verification

CI/automated tests should pass. No visual changes.

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 requested a review from a team as a code owner January 4, 2023 22:51
Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'd make sure that fluent tester still responds to light/dark OS changes properly if you haven't. Or High Contrast

@lyzhan7
Copy link
Contributor Author

lyzhan7 commented Jan 4, 2023

Looks good to me, I'd make sure that fluent tester still responds to light/dark OS changes properly if you haven't. Or High Contrast

Ooh thanks for bringing that up, I checked that iOS light/dark mode still looks right to me (looked at the shadows test page) - but actually think macOS might not work since that method doesn't have the appearance as a parameter to the method. If a memoized method doesn't take in any arguments then I think it will only ever be created once? Checking macOS now

@lyzhan7
Copy link
Contributor Author

lyzhan7 commented Jan 4, 2023

Double checked and confirmed that switching appearances in macOS is broken with this change, going to mark as a draft until I come up with a fix. Thinking I'll move the existing calls to getting the appearance up the call stack so that we pass the appearance through the method parameters.

Also this seems like something the PR checks should be able to catch, will also look into add more dark mode tests or maybe make a work item for this

@lyzhan7 lyzhan7 marked this pull request as draft January 4, 2023 23:50
@Saadnajmi
Copy link
Collaborator

Double checked and confirmed that switching appearances in macOS is broken with this change, going to mark as a draft until I come up with a fix. Thinking I'll move the existing calls to getting the appearance up the call stack so that we pass the appearance through the method parameters.

Also this seems like something the PR checks should be able to catch, will also look into add more dark mode tests or maybe make a work item for this

Oh yeah, we set up the macOS theme (and others) to call theme.invalidate() whenever the OS appearance changed, forcing a recreation of the theme object. You should be able to see that logic in createAppleTheme.macos.ts.

@lyzhan7
Copy link
Contributor Author

lyzhan7 commented Jan 5, 2023

  1. Updated macOS to get the mode when the theme is first created, and pass that object down to the creation of shadows/colors

    • Note - not really sure what mode vs. appearance refers to, but appearance is generally used as the name for the return value from getColorScheme() and mode is generally used as the name of the return value from getCurrentAppearance())
    • Confirmed that this gets macOS light/dark mode working after the memoize change
  2. I'm thinking of making the equivalent change for iOS/Android. I have the changes done locally, but wanted to get the macOS changes looked at first.

    • My understanding of the different platforms is that macOS supports light, dark, high contrast, and dynamic, while ios/android only support light/dark (for now). Does it make sense to be passing down an AppearanceOptions object in iOS/android the way macOS is now set up? Right now in some of the iOS/android code I think we've assumed that if the appearance isn't light, it's dark. In order to make the equivalent change for macOS on iOS/android there will be a couple places where we can potentially have an appearance that doesn't make sense (high contrast) - I can add the error handling for this but just wanted to check this is still the right approach to take.
    • For reference, here are some of the relevant types related to appearance:
// defined by react native
ColorSchemeName = 'light' | 'dark' | null | undefined;

// defined in theme-types
AppearanceOptions = 'light' | 'dark' | 'highContrast';
ThemeOptions[‘appearance’] = AppearanceOptions | ‘dynamic’;
  1. (Kind of related to 2) For iOS dark elevated colors, I wonder if we would we add that as an option to the AppearanceOptions interface? We'll run into a similar issue with other platforms where we could be setting a theme that doesn't exist on platform
  2. I'm thinking of removing appleShadows.macOS.ts/fallbackAppleShadows.ts altogether, and calling createMacOSShadowAliasTokens directly from AppleTheme.macOS.ts
  3. I actually don't think we have any dark mode tests (just doing a search for "dark" in *.test.tsx files - nothing comes up) right now - I'm hoping I can add a simple test that fails when the wrong token values are used for dark mode, for more thorough testing will make a work item for this

@lyzhan7 lyzhan7 marked this pull request as ready for review January 5, 2023 15:34
@lyzhan7 lyzhan7 merged commit f5de301 into microsoft:main Jan 26, 2023
@lyzhan7 lyzhan7 deleted the memoize-theme-creations branch January 26, 2023 18:09
@lyzhan7
Copy link
Contributor Author

lyzhan7 commented Jan 26, 2023

For 2, 4, and 5 above will follow up in a later PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants