-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
fix: theme is not configured in app router #16889
Conversation
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (10/01/24)1 reviewer was added to this PR based on Keith Williams's automation. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
E2E results are ready! |
test.describe("Change Theme Test", () => { | ||
test("change theme to dark", async ({ page, users }) => { | ||
const pro = await users.create(); | ||
testBothFutureAndLegacyRoutes.describe("Change Theme Test", () => { |
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.
adding testBothFutureAndLegacyRoutes
to test app router pages
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.
Can you turn APP_ROUTER_SETTINGS_MY_ACCOUNT_ENABLED
to 1
for dev? @keithwillcode
Otherwise, testBothFutureAndLegacyRoutes
will just test pages router twice
e7f9fec
to
8ae85c9
Compare
@@ -51,7 +51,7 @@ export function WithLayout<T extends Record<string, any>>({ | |||
requiresLicense={requiresLicense || !!(Page && "requiresLicense" in Page && Page.requiresLicense)} | |||
nonce={nonce} | |||
themeBasis={null} | |||
isThemeSupported={!!(Page && "isThemeSupported" in Page && Page.isThemeSupported)} | |||
isThemeSupported={Page && "isThemeSupported" in Page ? (Page.isThemeSupported as boolean) : undefined} |
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: I think isThemeSupported={Page?.isThemeSupported ?? undefined}
has the same effect here and is a lot more readable
isThemeSupported={Page && "isThemeSupported" in Page ? (Page.isThemeSupported as boolean) : undefined} | |
isThemeSupported=isThemeSupported={Page?.isThemeSupported ?? undefined} |
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.
Page?.isThemeSupported
will throw type error :( Feel free to make tweaks
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 - happy merging but i'd personally like to see the readability of that tenery improved
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist