-
Notifications
You must be signed in to change notification settings - Fork 1.5k
refactor(ThemeProvider): simplify the ThemeProvider #2970
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
Conversation
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.
Pull Request Overview
This PR refactors the theming logic by simplifying the ThemeProvider and consolidating the theming hook into the context, eliminating the need for a separate useThemeProvider hook. Key changes include:
- Removing the extra value prop from ThemeProvider and simplifying its API.
- Replacing imports from "@/utils/useAppTheme" with the new "@/theme/context" and adjusting type imports accordingly.
- Updating documentation and snapshots to reflect the new, simpler theming paradigm.
Reviewed Changes
Copilot reviewed 52 out of 53 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
test/vanilla/ignite-generate.test.ts | Updated import paths for theme values and type definitions. |
src/tools/markup.test.ts | Removed ThemeProvider value prop and updated imports to the new context. |
docs/* | Updated documentation to reflect the new ThemeProvider API and usage. |
boilerplate/* | Adjusted imports and removed obsolete hook usage across components. |
.github/CONTRIBUTING.md | Updated command reference for code formatting. |
Comments suppressed due to low confidence (2)
src/tools/markup.test.ts:547
- The ThemeProvider no longer accepts a 'value' prop. Please update its usage to simply render without passing any props.
<NavigationContainer ref={navigationRef} theme={navigationTheme} {...props}>
docs/boilerplate/app/theme/Theming.md:74
- Update the code example to reflect the new ThemeProvider API by removing the 'value' prop; it should be used as without additional parameters.
return (
688be86
to
30f103c
Compare
|
||
useBackButtonHandler((routeName) => exitRoutes.includes(routeName)) | ||
|
||
return ( | ||
<ThemeProvider value={{ themeScheme, setThemeContextOverride }}> |
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.
This ThemeProvider
instance has moved up a level so that the NavigationContainer
can get it's value from useAppTheme()
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.
Really like the refactors here, nice work!
export const useAppTheme = () => { | ||
const context = useContext(ThemeContext) | ||
if (!context) { | ||
throw new Error("useAppTheme must be used within an ThemeProvider") | ||
} | ||
return context | ||
} |
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.
Perfection
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.
+1
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.
Thanks for making these changes, @markrickert! I have one suggestion about the TypeScript here, but I wouldn't block on it. I ran the new code and everything lints, tests, builds, and works!
@@ -9,7 +9,7 @@ _Working on Ignite CLI requires Yarn v1._ | |||
- Everything works on iOS/Android | |||
- Jest tests pass in the root folder (`yarn test`) | |||
- New tests are included for any new functionality | |||
- Code is compliant with our linter and prettier (`yarn format && yarn lint`) | |||
- Code is compliant with our linter and prettier (`yarn format:write && yarn lint`) |
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.
praise: good catch, thanks!
@@ -42,16 +42,12 @@ Head on over to the [Ignite Cookbook](https://ignitecookbook.com/) to find recip | |||
|
|||
::: | |||
|
|||
**New in Ignite 10**: the `useAppTheme` hook allows you to create dynamically-themed styles, right out of the box. Here's an example: |
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.
praise: good update!
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.
Awesome work! No notes 💅
BREAKING CHANGE: adopt new architecture by default and remove legacy support (#2961 by @markrickert) - Enable New Architecture in all new projects; legacy option removed from CLI - Boilerplate and CLI now assume New Arch by default feat: upgrade to Expo SDK 53 with Android edge-to-edge support (#2938 by @fpena) - Upgrade to Expo SDK 53 and align all dependencies - Add support for Android edge-to-edge using SystemBars - Update app.json, jest-expo, metro.config, and related config for compatibility feat: remove mobx-state-tree and enable bring-your-own state management (#2960 by @markrickert) - Remove MST from boilerplate and CLI - Replace with lightweight React Context examples for demo state - Support persistence via useMMKVString feat: simplify theming with refactored ThemeProvider (#2970 by @markrickert) - Refactor useAppTheme and ThemeProvider for easier integration fix: address compatibility issues with React 19, TypeScript, and tests (#2966, #2968 by @objecttiveSee) - Solve TS 5.4/React 19 type issues and downgrade TS to 5.3.3 - Fix failing snapshots and metro test conditions - Add missing ESLint plugin and clean up demo code chore: update docs, config, and version alignment (#2964 by @frankcalise) - Sync README, quickstart, and context docs - Enforce node >= 20 and update linting rules across project
# [11.0.0](v10.5.3...v11.0.0) (2025-07-02) * Ignite v11 (Expo SDK 53) (#2938) ([154d2ed](154d2ed)), closes [#2938](#2938) [#2938](#2938) [#2960](#2960) [#2970](#2970) [#2966](#2966) [#2968](#2968) [#2964](#2964) ### Bug Fixes * **BackHandler:** switch to non-deprecated listener syntax [#2966](#2966) [skip ci] ([aeb3ee9](aeb3ee9)) * **types:** update useRef with default undefined value ([#2968](#2968)) [skip ci] ([1f7a8fe](1f7a8fe)) ### BREAKING CHANGES * adopt new architecture by default and remove legacy support (#2961 by @markrickert) - Enable New Architecture in all new projects; legacy option removed from CLI - Boilerplate and CLI now assume New Arch by default
Description
This refactors the theme section to be easier to use and use common react context patterns.
The functionality that used to be in
app/utils/useAppTheme.ts
and put it inapp/theme/context.ts
. I moved the code from the hook into the context so it's much easier to use and implement.Before:
After:
This paradigm also makes the
useAppTheme()
code stupid simple:This refactor also removes the barrel file from the
app/theme
and aligns the documentation to the new ThemeProvider instructions.Checklist
README.md
and other relevant documentation has been updated with my changes