Skip to content

refactor: remove theme "color palettes" #11314

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

Merged
merged 5 commits into from
Dec 21, 2023
Merged

refactor: remove theme "color palettes" #11314

merged 5 commits into from
Dec 21, 2023

Conversation

aslilac
Copy link
Member

@aslilac aslilac commented Dec 21, 2023

buffet style is super bug prone, much better to have curated colors with specific purposes

@Parkreiner Parkreiner self-requested a review December 21, 2023 21:12
},
},
} as ThemeOptions);

Copy link
Member

Choose a reason for hiding this comment

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

Big fan of getting rid of all this

Copy link
Member

@Parkreiner Parkreiner 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! Big fan of the consolidation for the MUI-specific stuff
Just had one comment about possibly removing the need for any types

@@ -1,3 +1,18 @@
/* eslint-disable @typescript-eslint/no-explicit-any
-- we need to hack around the MUI types a little */
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we can get a little more specific with the type assertions by doing something like this:

type MuiOverrideStyleKey = keyof ComponentNameToClassKey;

styleOverrides: {
  ["sizeXlarge" as MuiOverrideStyleKey]: {...}
}

I pulled down the branch and tried it out, and it seems like it silences the compiler warnings. You know more about the theming set up than I do, though. I'm normally pretty skittish around any types, but do you think a change like this is even worth it?

Copy link
Member Author

@aslilac aslilac Dec 21, 2023

Choose a reason for hiding this comment

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

my concern about that is that it's technically lying to the compiler 😅 we'd be casting it as something it is not, and it would read as "trust me, I know what I'm doing", whereas using any is hinting a bit more at "this should make you nervous because something going on here is wrong and we should maybe fix it more rigorously later"

Copy link
Member Author

Choose a reason for hiding this comment

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

(I genuinely intend to improve this in the new year. I hope I've earned a bit of trust that I'm the type of dev who will actually go back and clean up debt, lol.)

Copy link
Member

@Parkreiner Parkreiner Dec 21, 2023

Choose a reason for hiding this comment

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

I guess the way I see it is that both ways are lying to the compiler, but only the any approach completely turns the compiler off

I'm cool if you want to keep things as-is, but if anything, I feel like the way to split the difference would be to rename the type to something like HackyMuiOverrideStyleKey

Copy link
Member

@Parkreiner Parkreiner Dec 21, 2023

Choose a reason for hiding this comment

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

Oh, sorry. For some reason your second comment didn't come in when I was replying
To be clear, I do trust you – I didn't realize that this was a temp solution, and I'm being an ass

@aslilac aslilac merged commit db71c0f into main Dec 21, 2023
@aslilac aslilac deleted the theme-refactors branch December 21, 2023 21:45
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants