Conversation
…tup is initialized on user login
src/apps/renderer/hooks/useTheme.tsx
Outdated
| const updateTheme = (newTheme: Theme) => { | ||
| setTheme(newTheme); | ||
| applyThemeClass(newTheme); | ||
| }; |
There was a problem hiding this comment.
if you dont use arrow functions there is no need in adding this inside the useEffect or add them as a dependency
src/apps/renderer/hooks/useTheme.tsx
Outdated
| applyThemeClass(newTheme); | ||
| }; | ||
|
|
||
| const resolveTheme = (configTheme: ConfigTheme) => { |
| @@ -0,0 +1,61 @@ | |||
| import { useEffect, useState } from 'react'; | |||
There was a problem hiding this comment.
Remember to add tests to this. If necessary, extract the functions outside the hook (So that it is easier for you to test them) and wrap it inside a folder if necessary
| @@ -1,30 +1,47 @@ | |||
| import { ReactNode } from 'react'; | |||
There was a problem hiding this comment.
Since you are changing the component and it is not tested, we need to add test so that the next time it recieves changes, we know that it does not break anything
| size?: 'sm' | 'md' | 'lg' | 'xl' | '2xl'; | ||
| children: ReactNode; | ||
| customClassName?: string; | ||
| disabled?: boolean; | ||
| } | ||
|
|
||
| export default function Button(props: ButtonProps) { | ||
| const [isExecuting, setIsExecuting] = useState(false); |
There was a problem hiding this comment.
It is way easier to let the consumer of this component have the responsability of setting if is executing or not. Pass down the isExecuting as a prop and not as a useState
| @@ -0,0 +1,23 @@ | |||
| import { OnboardingSlideProps } from '../helpers'; | |||
There was a problem hiding this comment.
add a test for this since you delete the other component and his test
src/apps/renderer/hooks/useTheme.tsx
Outdated
| @@ -0,0 +1,61 @@ | |||
| import { useEffect, useState } from 'react'; | |||
| import { ConfigTheme, Theme, ThemeData } from '../../shared/types/Theme'; | |||
There was a problem hiding this comment.
Also, we could think this through and add a common context for this sort of things, such as theme, language, maybe user products..etc.
So that we only have one state for this (Since a global, common state for these values is enough)
There was a problem hiding this comment.
It's not worth doing right now. The components that use the theme in such a complex way at this point are only those involved in onboarding.
|
|
||
| export const BackupsSlide: React.FC<OnboardingSlideProps> = () => { | ||
| const { translate } = useTranslationContext(); | ||
| const { theme } = useTheme(); |
There was a problem hiding this comment.
An argument in favor of common contet: This way we dont duplicate states.
|
2 participants
What is Changed / Added
The necessary changes have been made to adjust the application's onboarding to the design that should be used on all platforms.