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

Fix broken theming #1834

Merged
merged 3 commits into from
Mar 30, 2023
Merged

Fix broken theming #1834

merged 3 commits into from
Mar 30, 2023

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Mar 29, 2023

Status: Still going to add a test so leaving this PR in draft until then.

Closes #1814.

Themes were not working - this fix seems to work.

Screenshot 2023-03-29 at 20 05 14

The regression was introduced in #1627 - it looks like deep merging the palette like that does not work, so we have to override it in the theme object? @Janpot

@apedroferreira apedroferreira added bug 🐛 Something doesn't work regression A bug, but worse labels Mar 29, 2023
@apedroferreira apedroferreira requested a review from Janpot March 29, 2023 19:05
@apedroferreira apedroferreira self-assigned this Mar 29, 2023
@apedroferreira
Copy link
Member Author

Wait I can probably add a test too. Will do in this PR.

@apedroferreira apedroferreira marked this pull request as draft March 29, 2023 19:08
@apedroferreira apedroferreira removed the request for review from Janpot March 29, 2023 20:13
fontSize: `3.25rem`,
fontWeight: 800,
},
return createTheme(
Copy link
Member

@Janpot Janpot Mar 30, 2023

Choose a reason for hiding this comment

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

Would just the following work? Not sure why I did that, but it doesn't make a lot of sense to me.

createTheme({
  palette,
  typography: {
    // ...
  }
})

@Janpot
Copy link
Member

Janpot commented Mar 30, 2023

Wait I can probably add a test too.

👍 Just a smoke test of an app with a dark theme should be fine. Not that useful to add tests for the theme editor at this point I believe.

@apedroferreira apedroferreira marked this pull request as ready for review March 30, 2023 17:27
@apedroferreira apedroferreira requested a review from Janpot March 30, 2023 17:27
@@ -75,6 +77,8 @@ export class ToolpadEditor {
this.componentCatalog = page.getByTestId('component-catalog');
this.componentEditor = page.getByTestId('component-editor');

this.themeEditor = page.getByTestId('theme-editor');
Copy link
Member

Choose a reason for hiding this comment

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

I meant to not test the theme editor, but ok 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, you meant just starting with dark theme... I thought you meant testing light/dark theme was ok but not UI colors as those could change more later.
Anyway even if we change things later I guess some of these could still be useful, if not we'll eventually delete some of this and that's fine.

@apedroferreira apedroferreira merged commit 4e7727e into master Mar 30, 2023
@apedroferreira apedroferreira deleted the fix-theming branch March 30, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix theming
2 participants