Skip to content

feat: [ts-starter #6] implement theming infrastructure#10

Open
ehsanmmd wants to merge 3 commits into
mainfrom
feat/integrate-theming-infrastructure
Open

feat: [ts-starter #6] implement theming infrastructure#10
ehsanmmd wants to merge 3 commits into
mainfrom
feat/integrate-theming-infrastructure

Conversation

@ehsanmmd

Copy link
Copy Markdown
Contributor

No description provided.

@ehsanmmd
ehsanmmd requested a review from momesana March 15, 2024 09:21
@ehsanmmd ehsanmmd self-assigned this Mar 15, 2024
Comment thread src/pages/test.tsx Outdated
</option>
))}
</select>
<Typography>

@momesana momesana Mar 15, 2024

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

  • That's a weird name and highly confusing given that antd it'self has a component named like that. Name it TypographySample instead.

Comment thread src/components/ThemeProvider/theme.ts Outdated
...defaultTheme,
colorText: "#ffffff",
colorBgContainer: "#000000",
colorBgElevated: "#333333",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'd think that all but colorWhite also need adapting here i. e. also colorTextPrimary and colorTextSecondary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

return (
<ThemeContext.Provider
value={{
selectTheme,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

  • memoize value

Passing value unmemoized is a horrible thing to do. Every component that makes use of the theme object will be rerendered everytime App.tsx is rerendered because of this. value absolutely must be memoized for that not to happen! You need to know where not memoizing things will cascade down and cause a lot of unnecessary rerenders. Contexts, especially those high up the react tree are one of those candidates.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The selectTheme function has been created using useCallback. I thought that should be enough to prevent the reference from changing.

Comment thread src/components/ThemeProvider/index.tsx Outdated
};

useEffect(() => {
selectTheme(getStoredTheme());

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

  • Why do you use useEffect here?

@ehsanmmd ehsanmmd Mar 16, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I needed to apply the loaded theme from the local storage and It's supposed to run just once

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed the code

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