-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(makeStyles): return theme that is extended #3473
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(makeStyles): return theme that is extended #3473
Conversation
| return useMemo(() => { | ||
| const css = | ||
| typeof styles === 'function' | ||
| ? styles({ colors: theme.colors, mode: theme.mode }, props) |
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.
when making a custom theme, the theme shape could look like, for example:
colors: {...default RNE colors},
mode: {...default RNE mode},
myCustomColors: {
text: {
primary: string;
secondary: string;
}
}
when consuming makeStyles for a component, the current implementation of the styles() here ignores any custom fields that have been extended from the FullTheme.
This change proposes returning the full theme so that users have access to their custom/extended themes.
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.
We can extend the Colors itself, IMO, having complete theme object is not necessary for makeStyles
Use never to remove existing colors if they aren't required.
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 explain further how this would work?
I ask because when we define an extended theme (as per #3450), in our new themed.d.ts file, the custom "theme" name is not a known name (unless RNE adds a specific field to be used for this purpose). I couldn't think how else to guarantee a return of an unknown field name, except to return the entire theme, which is extended off of FullTheme:
declare module '@rneui/themed' {
export interface FullTheme {
colors: RecursivePartial<Colors>;
fluidColors: ColorShape;
}
}
As you can see below, makeStyles() does not return the full theme since it only returns theme.colors, mode: theme.mode and is missing theme.fluidColors
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 try #3472 (comment) ?
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.
I understood your point, but what I want to say is, having complete theme object will also contain default Component Props, which I think is overkilling the makestyles, because we only need colors/mode for styling, I believe. Also in your case we only need to add some more colors which can be done by extending Colors.
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.
#3472 solves the issue. thank you
Codecov Report
@@ Coverage Diff @@
## next #3473 +/- ##
==========================================
+ Coverage 78.25% 78.28% +0.03%
==========================================
Files 87 87
Lines 1775 1773 -2
Branches 786 784 -2
==========================================
- Hits 1389 1388 -1
+ Misses 381 380 -1
Partials 5 5
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |

Motivation
As reported in #3472, when creating an extended theme,
makeStylesdoes not return the full extended them at run time.This is a followup to #3450.
Fixes # (issue) #3472
Type of change
How Has This Been Tested?
themed.d.tsfile for a testexampleappChecklist
yarn docs-build-api. --- n/aAdditional context