Skip to content

Conversation

@lucksp
Copy link
Contributor

@lucksp lucksp commented Apr 26, 2022

Motivation

As reported in #3472, when creating an extended theme, makeStyles does not return the full extended them at run time.

This is a followup to #3450.

Fixes # (issue) #3472

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Jest Unit Test - unable to verify with Jest using a custom themed.d.ts file for a test
  • Checked with example app

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation using yarn docs-build-api. --- n/a
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works --- see note above
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Additional context

return useMemo(() => {
const css =
typeof styles === 'function'
? styles({ colors: theme.colors, mode: theme.mode }, props)
Copy link
Contributor Author

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.

Copy link
Member

@arpitBhalla arpitBhalla Apr 27, 2022

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.

Copy link
Contributor Author

@lucksp lucksp Apr 27, 2022

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

image

Copy link
Member

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) ?

Copy link
Member

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.

Copy link
Contributor Author

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

@lucksp lucksp changed the title Fix/pl makestyles return theme (Fix) Makestyles return theme that is extended Apr 26, 2022
@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #3473 (444c50d) into next (587d8db) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            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              
Impacted Files Coverage Δ
packages/themed/src/config/makeStyles.ts 90.00% <100.00%> (+6.66%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@arpitBhalla arpitBhalla changed the title (Fix) Makestyles return theme that is extended fix(makeStyles): return theme that is extended Apr 27, 2022
@lucksp lucksp closed this Apr 27, 2022
@lucksp lucksp deleted the fix/pl-makestyles-return-theme branch April 27, 2022 18:25
@lucksp lucksp mentioned this pull request Apr 27, 2022
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants