chore: remove typography selection in app theming #36110
Conversation
WalkthroughThis update introduces several enhancements to the design system's theming hooks, focusing on optimizing performance by replacing state management with memoization. Key hooks like Changes
Assessment against linked issues
Tip Announcements
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review due to trivial changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
jsartisan
changed the title
github-actions
bot
added
the
skip-changelog
app/client/packages/design-system/theming/src/hooks/src/useIconDensity.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- app/client/packages/design-system/theming/src/hooks/src/index.ts (1 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useIconDensity.tsx (1 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useIconSizing.tsx (1 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useSizing.tsx (3 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useSpacing.tsx (2 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useTheme.tsx (4 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useTypography.tsx (2 hunks)
- app/client/src/pages/Editor/Canvas.tsx (1 hunks)
- app/client/src/pages/Editor/WDSThemePropertyPane/index.tsx (4 hunks)
Additional comments not posted (16)
app/client/packages/design-system/theming/src/hooks/src/index.ts (2)
6-6: Great work on adding theuseIconDensityexport! 👍This new export enhances the functionality of the module by providing a custom hook for managing icon density within the design system. It can be utilized in other parts of the application to improve the flexibility and customization of icon rendering.
The export is properly declared and follows the existing pattern of the module.
7-7: Excellent addition of theuseIconSizingexport! 🌟This new export further enhances the functionality of the module by providing a custom hook for managing icon sizing within the design system. It can be utilized in other parts of the application to improve the flexibility and customization of icon sizing.
The export is properly declared and follows the existing pattern of the module.
app/client/packages/design-system/theming/src/hooks/src/useIconSizing.tsx (1)
Line range hint
1-21: Great job refactoring theuseIconSizinghook! The changes have improved performance and readability. 👍The refactoring has several benefits:
By replacing
useStateanduseEffectwithuseMemo, the hook now directly computes the icon size based on theuserSizingandsizingparameters. This eliminates the need for state updates and side effects, reducing unnecessary re-renders and improving performance.The use of
useMemois appropriate here because the icon size only needs to be recomputed when theuserSizingorsizingparameters change. This ensures that the computed value is memoized and reused unless the dependencies change.The conditional statements in the
useMemocallback cover all possible cases foruserSizingand return the appropriate icon size based on the providedsizingobject. This makes the logic clear and easy to understand.The hook has a clear and concise implementation, with a single responsibility of computing the icon size based on the provided parameters.
Overall, the refactoring has resulted in a more efficient and maintainable hook. Keep up the good work!
app/client/packages/design-system/theming/src/hooks/src/useIconDensity.tsx (1)
Line range hint
1-21: Great job refactoring theuseIconDensityhook! The changes look excellent. 👍I really like how you've simplified the logic by replacing
useStateanduseEffectwithuseMemo. This change not only improves performance by avoiding unnecessary re-renders but also makes the code more readable and maintainable.The conditional statements used to determine the value of
strokeWidthbased onuserDensityare clear and concise. It's easy to understand the flow of the logic and how the appropriate value is returned.Additionally, specifying the type of
userDensityasnumberin the hook signature is a nice touch. It provides clarity and helps catch potential type-related issues.Overall, these changes enhance the efficiency and maintainability of the
useIconDensityhook. Well done!app/client/packages/design-system/theming/src/hooks/src/useSizing.tsx (2)
11-11: Great job explicitly defining the return type ofgetSizing! 👍Specifying the return type as
TokenObjimproves type safety and makes the code more self-documenting. This change enhances the clarity and maintainability of the codebase.
38-40: Excellent refactoring of theuseSizinghook! 🌟By replacing
useStateanduseEffectwithuseMemo, you have optimized the performance of the hook. Thesizingvalue is now memoized based on its dependencies (config,userDensity, anduserSizing), ensuring that it is recalculated only when necessary.This change reduces unnecessary computations and improves the rendering efficiency of components that utilize the
useSizinghook. Well done on implementing this performance optimization!app/client/packages/design-system/theming/src/hooks/src/useSpacing.tsx (2)
39-41: Great use ofuseMemofor performance optimization! 👍By wrapping the
getSpacingfunction call withuseMemo, you are ensuring that theouterSpacingvalue is only recalculated when one of its dependencies (outerSpacingConfig,userDensity, oruserSizing) changes. This memoization technique helps avoid unnecessary re-computations and improves the overall performance of the component.Keep up the good work in leveraging React's capabilities effectively!
43-45: Excellent application ofuseMemoforinnerSpacingas well! 🌟Just like with
outerSpacing, you have effectively utilizeduseMemoto memoize the result of thegetSpacingfunction call forinnerSpacing. By specifying the dependencies asinnerSpacingConfig,userDensity, anduserSizing, you ensure that the memoized value is only recalculated when necessary.This optimization technique enhances the efficiency of the component by reducing unnecessary re-renders and improving performance. Well done on consistently applying this concept throughout the code!
app/client/packages/design-system/theming/src/hooks/src/useTypography.tsx (1)
67-69: Great job refactoring theuseTypographyhook! The changes look good to me.The refactor improves the efficiency of the hook by:
- Replacing
useStateanduseEffectwithuseMemoto avoid unnecessary re-renders and state updates.- Calculating the typography value synchronously based on the dependencies of
config,userDensity, anduserSizing.Additionally, removing the
fontFamilyparameter from the function signature simplifies the API for users of this hook, indicating a shift in how typography is configured.Overall, the changes enhance performance and streamline the logic for typography calculation. Well done!
app/client/packages/design-system/theming/src/hooks/src/useTheme.tsx (5)
2-2: Great job updating the imports!The changes to the imports are necessary to support the refactoring of the
useThemefunction. The addition ofuseMemoand the new hooksuseIconSizinganduseIconDensitywill help improve the performance and maintainability of the code.Also applies to: 4-10
56-128: Excellent refactoring of theuseThemefunction! 👏The replacement of multiple
useEffecthooks with a singleuseMemohook is a great way to improve performance and reduce the number of side effects. By consolidating the theme update logic into theuseMemofunction, you've made the code more readable and maintainable.The
useMemohook will ensure that the theme is only recalculated when the relevant dependencies change, which will help optimize the performance of the component.
79-82: Nice work streamlining the typography handling!The integration of typography updates directly into the memoized theme calculation is a great way to simplify the theme management process. By removing the separate effect for updating typography, you've made the code more cohesive and easier to understand.
This change also ensures that typography updates are applied consistently whenever the theme is recalculated.
Also applies to: 108-108
104-116: Great addition of icon sizing and stroke width to the theme object!Including the icon sizing and stroke width properties in the returned theme object is a helpful change that ensures these values are easily accessible for components that require them.
This change aligns well with the addition of the new hooks
useIconSizinganduseIconDensity, which provide a more granular control over icon-related properties in the theme.
Line range hint
18-24: Verify the impact of removing thefontFamilyproperty.The removal of the
fontFamilyproperty from theUseThemePropsinterface suggests a change in how typography is managed. Please ensure that this change does not break any existing code that relies on thefontFamilyproperty.You can use the following script to search for any usage of the
fontFamilyproperty in the codebase:app/client/src/pages/Editor/Canvas.tsx (1)
Line range hint
55-55: Verify that removing thefontFamilyproperty is intentional.Removing the
fontFamilyproperty from thethemeSettingobject may affect the visual presentation of text within theCanvascomponent, as it no longer has a specified font family. This could lead to a fallback to a browser default or inherited styles.The AI-generated summary indicates that this change is part of simplifying typography management. Can you please confirm that this change is intentional and aligns with the broader typography management goals mentioned in the summary?
app/client/src/pages/Editor/WDSThemePropertyPane/index.tsx (1)
15-15: Looks good! The import statement for components from "@appsmith/ads" is correctly added.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10697206787. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/packages/design-system/theming/src/hooks/src/useIconDensity.tsx (1 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useIconSizing.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/theming/src/hooks/src/useIconSizing.tsx
Additional comments not posted (2)
app/client/packages/design-system/theming/src/hooks/src/useIconDensity.tsx (2)
Line range hint
1-20: Great job refactoring the hook to improve performance! 👏The changes simplify the logic and avoid unnecessary re-renders by replacing
useStateanduseEffectwithuseMemo. This is a significant improvement that enhances the efficiency of the hook.
Line range hint
1-20: Acknowledging the previous review comments.The conversation in the previous review comments provides valuable context for understanding the motivation behind the refactoring. It's great to see that the feedback was taken into account and the code was updated accordingly.
|
Deploy-Preview-URL: https://ce-36110.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/packages/design-system/theming/src/hooks/src/useTheme.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/theming/src/hooks/src/useTheme.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/client/packages/design-system/theming/src/hooks/src/useTheme.tsx (4 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useTypography.tsx (2 hunks)
- app/client/packages/storybook/.storybook/addons/theming/ThemingPopup.tsx (1 hunks)
- app/client/packages/storybook/.storybook/decorators/theming.tsx (1 hunks)
- app/client/packages/storybook/src/components/ThemeSettings.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/theming/src/hooks/src/useTheme.tsx
- app/client/packages/design-system/theming/src/hooks/src/useTypography.tsx
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10734651305. |
|
Deploy-Preview-URL: https://ce-36110.dp.appsmith.com |
|
@ichik can you try the DP once? For some reason, there are diffs in few components in storybook. Can you check if they are ok to go in ? |
|
@jsartisan everything seems to be fine, minor changes in Chromatic are a mystery, I don't see them when inspecting Storybook snapshots, there appears to be a minor vertical positioning difference, though, perhaps some rounding did change somewhere. Behavior and looks in the editor seem all fine. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsx (1)
57-87: Ensure Complete Dependency Arrays in useMemo HooksThe
useMemohooks forcolorClassNameandtypographyClassNamedo not have explicit dependencies listed. This could lead to issues with stale or incorrect data if these hooks depend on other variables. Please verify if additional dependencies should be included for these hooks.
colorClassName: Consider adding[color]as a dependency.typographyClassName: Consider adding[typography]as a dependency.Analysis chain
Optimization through useMemo
The use of
useMemoforcolorClassName,typographyClassName,fontFamilyClassName,providerClassName, andcolorModeClassNameis a significant improvement. It ensures that these values are recalculated only when their dependencies change, which enhances performance by preventing unnecessary re-renders.However, it's crucial to ensure that all dependencies are correctly listed in the dependency arrays of
useMemoto avoid issues with stale or incorrect data.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all dependencies are correctly listed for useMemo hooks. # Test: Check the dependency arrays for completeness and correctness. rg --type javascript -A 3 $'useMemo(() => {'Length of output: 139
Script:
#!/bin/bash # Description: Verify that all dependencies are correctly listed for useMemo hooks. # Test: Check the dependency arrays for completeness and correctness in the specified file. rg --type js -A 5 'useMemo\(\(\) => {' app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsxLength of output: 213
Script:
#!/bin/bash # Description: Verify that all dependencies are correctly listed for useMemo hooks. # Test: Check the dependency arrays for completeness and correctness in the specified file. rg --type js -A 5 'useMemo\(\[\w*,?\s*\w*\]\)\s*=>\s*\{' app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsxLength of output: 131
Script:
#!/bin/bash # Description: Verify that all dependencies are correctly listed for useMemo hooks. # Test: Check the dependency arrays for completeness and correctness in the specified file. rg --type js -A 5 'useMemo\(\s*\(\)\s*=>\s*\{' app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsxLength of output: 666
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsx (2 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useTheme.tsx (4 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useTypography.tsx (4 hunks)
- app/client/packages/design-system/theming/src/theme/src/ThemeProvider.tsx (1 hunks)
- app/client/packages/design-system/theming/src/theme/src/types.ts (1 hunks)
- app/client/packages/design-system/theming/src/token/src/TokensAccessor.ts (7 hunks)
- app/client/packages/design-system/theming/src/token/src/types.ts (4 hunks)
- app/client/src/pages/AppViewer/index.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
- app/client/packages/design-system/theming/src/token/src/TokensAccessor.ts
Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/theming/src/hooks/src/useTheme.tsx
- app/client/packages/design-system/theming/src/hooks/src/useTypography.tsx
Additional comments not posted (8)
app/client/packages/design-system/theming/src/theme/src/types.ts (1)
2-2: Repositioning of CSSProperties import is a good practice.Moving the
CSSPropertiesimport to a more prominent position at the top enhances the organization of imports, making it easier to locate and manage dependencies. Well done on this subtle yet effective improvement!app/client/packages/design-system/theming/src/theme/src/ThemeProvider.tsx (1)
23-23: Optimized handling of thethemeobject inuseCssTokens.Passing the
themeobject directly to theuseCssTokenshook instead of using a spread operator is a wise choice. This approach reduces the overhead of creating a shallow copy of the object, which can enhance performance, especially in complex applications. However, please confirm that theuseCssTokensfunction is equipped to handle direct object references without any side effects.Verification successful
Let's try a different approach to locate the
useCssTokensfunction. I'll search for its occurrences across the codebase to determine where it's defined or imported from. This will help us understand how it handles direct object references.
Direct object reference handling in
useCssTokensis confirmed.The
useCssTokensfunction is designed to accept aThemeobject directly, as evidenced by its implementation inuseCssTokens.tsx. The function destructures theThemeobject properties, ensuring it can handle direct references without issues. This confirms that the change inThemeProvider.tsxis both safe and efficient.
- Location of
useCssTokensimplementation:app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsxScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `useCssTokens` handles direct object references correctly. # Test: Search for the implementation of `useCssTokens` to ensure it handles direct object references. Expect: Proper handling of direct references. ast-grep --lang typescript --pattern $'function useCssTokens($_) { $$$ }'Length of output: 76
Script:
#!/bin/bash # Search for occurrences of `useCssTokens` in the codebase to find its definition or import statement. rg 'useCssTokens' -A 5Length of output: 2170
app/client/packages/design-system/theming/src/token/src/types.ts (2)
Line range hint
7-7: Removal offontFamilyfromTokenSource.Eliminating the
fontFamilyproperty from theTokenSourceinterface is a bold move towards simplifying the typography settings within the application. This change should make the theme configuration less complex and more maintainable. However, it's crucial to ensure that all components and features that previously relied on this property have been appropriately adjusted to work without it.
Line range hint
7-7: Removal ofFONT_METRICSandFontFamilytype.The removal of the
FONT_METRICSconstant and theFontFamilytype indicates a significant shift in how fonts are managed within the system. This could lead to a more centralized control over typography, potentially simplifying the theming system. Ensure that the removal of these elements does not impact the functionality of the application, particularly in areas where font metrics were previously utilized.app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsx (2)
9-11: Simplified fontFamilyCss functionThe
fontFamilyCssfunction has been simplified to always return a default font family string. This change aligns with the PR's goal to streamline typography settings. However, ensure that this default setting meets all design requirements across the application.
Line range hint
18-26: Review of dynamic CSS generation in getTypographyCssThe function dynamically generates CSS based on typography settings using template literals and
reduce. This method is efficient for creating customizable styles but ensure that the keys used intypographyare sanitized and validated to prevent CSS injection risks.app/client/src/pages/AppViewer/index.tsx (2)
49-50: Introduction of new types for theme propertiesThe introduction of
ColorModeandIconStyletypes is a positive change that enhances type safety and clarity. Ensure that these types are well-defined and cover all necessary cases in theme management.
114-117: Type casting in theme property assignmentsThe explicit casting of
themeSetting.colorModeandthemeSetting.iconStyletoColorModeandIconStyle, respectively, is a good practice that prevents type mismatches and potential bugs. However, ensure that the values provided bythemeSettingare compatible with these types and that there are fallbacks or validations in place in case of mismatches.
| ThemeToken, | ||
| IconStyle, | ||
| } from "../../token"; | ||
| import type { Typography, ThemeToken, IconStyle } from "../../token"; | ||
|
|
||
| export type Theme = ThemeToken & { |
There was a problem hiding this comment.
Tip
Codebase Verification
Review and Update References to fontFamily
The removal of the fontFamily property from the Theme type has not been fully propagated throughout the codebase. There are numerous references to fontFamily in various files, including widget styles, tests, migrations, and other components. It's important to review and update these references to ensure consistency and prevent potential errors.
- Files with
fontFamilyreferences:app/client/src/globalStyles/WidgetGlobalStyles.tsapp/client/packages/dsl/src/migrate/tests/SelectWidget.test.tsapp/client/src/widgets/TextWidget/component/index.tsxapp/client/src/pages/AppViewer/index.tsxapp/client/src/entities/AppTheming/index.ts- ...and many more.
Please ensure that all these references are updated or removed as necessary to align with the changes in the Theme type. This will help maintain a streamlined and consistent theme configuration across the application.
Analysis chain
Removal of fontFamily from Theme type.
The decision to remove the fontFamily property from the Theme type is a significant change. It simplifies the theme configuration by reducing the number of customizable options, which aligns with the PR's goal to streamline typography settings. Ensure that all references to fontFamily in the application have been updated to reflect this change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that no remaining references to `fontFamily` exist in the project.
# Test: Search for `fontFamily` in the project. Expect: No occurrences outside of historical or documentation contexts.
rg --type ts --type tsx "fontFamily"
Length of output: 64
Script:
#!/bin/bash
# Description: Verify that no remaining references to `fontFamily` exist in the project.
# Test: Search for `fontFamily` in TypeScript and TSX files. Expect: No occurrences outside of historical or documentation contexts.
rg "fontFamily" --glob "**/*.ts" --glob "**/*.tsx"
Length of output: 55381
|
|
||
| export const useIconDensity = (density: IconDensity, userDensity = 1) => { | ||
| const [strokeWidth, setStrokeWidth] = useState<TokenObj>(); |
There was a problem hiding this comment.
Note: In all these hooks, mainly refactored useState + useEffect pattern to useMemo.
Reason: useState + useEffect create 1 extra render.
| @@ -107,21 +108,16 @@ function AppViewer(props: Props) { | |||
| ); | |||
| const isAnvilLayout = useSelector(getIsAnvilLayout); | |||
| const themeSetting = useSelector(getAppThemeSettings); | |||
| const themeProps = { | |||
There was a problem hiding this comment.
Not sure why were we passing the themeProps for non-anvil apps 🤔
@KelvinOm do you remember?
I think just passing the empty object would be better for non-anvil apps.
There was a problem hiding this comment.
Nope. Looks like mistake.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsx (2 hunks)
Additional comments not posted (3)
app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsx (3)
9-11: SimplifiedfontFamilyCssfunction.The refactoring of
fontFamilyCssto directly return a string without parameters simplifies the code and aligns with the PR's objective to streamline typography settings. This change enhances maintainability by reducing complexity.
57-57: Optimized state management usinguseMemo.The transition from
useStateanduseEffecttouseMemofor managing CSS class names is a significant improvement. This change reduces unnecessary re-renders and enhances performance by recalculating values only when necessary. The implementation is clean and adheres to best practices in React for optimizing performance.Also applies to: 59-63, 67-71, 75-77, 79-82, 85-89
Line range hint
18-26: Review the dynamic CSS generation ingetTypographyCss.The use of
objectKeysto iterate over typography properties and dynamically generate CSS rules is efficient and flexible. However, ensure that theTypographytype is strictly typed to avoid runtime errors due to unexpected property access.Verification successful
Typography Type is Strictly Typed
TheTypographytype is strictly typed, as it derives its keys from the well-definedTYPOGRAPHY_VARIANTSconstant. This ensures that the dynamic CSS generation ingetTypographyCssis safe from runtime errors due to unexpected property access. The keys are limited tofootnote,caption,body,subtitle, andtitle, ensuring strict typing.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the strict typing of the Typography type. # Test: Search for the Typography type definition. Expect: Properties should be strictly defined. ast-grep --lang typescript --pattern $'type Typography = { $$$ }'Length of output: 373
Script:
#!/bin/bash # Description: Verify the definition of TYPOGRAPHY_VARIANTS to ensure strict typing. # Test: Search for the definition of TYPOGRAPHY_VARIANTS. Expect: It should be a constant or a type with specific keys. rg 'TYPOGRAPHY_VARIANTS' -A 5Length of output: 10152
jsartisan
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
|
|
||
| return { theme, setTheme }; | ||
| return { | ||
| ...tokensAccessor.getAllTokens(), |
There was a problem hiding this comment.
Is all this updated correctly? Have you checked?
There was a problem hiding this comment.
Yep. Looks ok to me.
| @@ -107,21 +108,16 @@ function AppViewer(props: Props) { | |||
| ); | |||
| const isAnvilLayout = useSelector(getIsAnvilLayout); | |||
| const themeSetting = useSelector(getAppThemeSettings); | |||
| const themeProps = { | |||
There was a problem hiding this comment.
Nope. Looks like mistake.
| userSizing: themeSetting.sizing, | ||
| userDensity: themeSetting.density, | ||
| iconStyle: themeSetting.iconStyle.toLowerCase(), | ||
| iconStyle: themeSetting.iconStyle.toLowerCase() as IconStyle, |
There was a problem hiding this comment.
Why now we need type casting?
There was a problem hiding this comment.
That's just how typescript is.
Consider the following code:
const isFlagTrue = localStorage.getItem('flag')
const a = { one: "one" as string, two: "two" as "two" };
const b = { two: "two" } as const;
function process(value: { one?: "one" | "ONE", two?: "two" | "two" }) {
}
process(isFlagTrue ? a : b); // works fine
In the above, when checking types for args of processt, typescript gets strict only on the types that are common to a and b ( type of two ).
Now consider the following case in which else of part of condition is return is empty object {}:
process(isFlagTrue ? a : {}); // ERROR, value of one should "one" or "ONE"
Now typescript is strict on the types of a. ( this is what is happening to us in our case, so had to typescript the wdsThemeProp properly )
Now the funny part:
If I extract the logic to a separate line, all works fine:
const arg = isFlagTrue ? a: {};
process(arg);
Not sure why this happens, will check if I can find something on this.
/ok-to-test tags="@tag.All" <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced new hooks for icon management: `useIconDensity` and `useIconSizing`. - **Improvements** - Refactored hooks to enhance performance by using `useMemo` instead of state management, leading to more efficient calculations for icon size, density, spacing, and typography. - Streamlined theme management by consolidating logic and removing unnecessary properties, improving clarity and maintainability. - Optimized CSS class name management within the `useCssTokens` hook for better performance. - **Bug Fixes** - Removed `fontFamily` property from various components, which may affect text rendering but simplifies font management. - **Documentation** - Updated comments and documentation to reflect changes in typography and theme handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10786030048> > Commit: a0d1258 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10786030048&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Tue, 10 Sep 2024 06:00:30 UTC <!-- end of auto-generated comment: Cypress test results --> --------- Co-authored-by: Pawan Kumar <pawankumar@Pawans-MacBook-Pro-2.local>
/ok-to-test tags="@tag.All"
Summary by CodeRabbit
Summary by CodeRabbit
New Features
useIconDensityanduseIconSizing.Improvements
useMemoinstead of state management, leading to more efficient calculations for icon size, density, spacing, and typography.useCssTokenshook for better performance.Bug Fixes
fontFamilyproperty from various components, which may affect text rendering but simplifies font management.Documentation
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10786030048
Commit: a0d1258
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 10 Sep 2024 06:00:30 UTC