Skip to content
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

feat: Add CozyTheme provider to handle normal and inverted themes and adapt existing UI #1062

Merged
merged 11 commits into from
Dec 13, 2023

Conversation

Ldoppea
Copy link
Member

@Ldoppea Ldoppea commented Dec 11, 2023

Tip

As usual this PR is meant to be read commit by commit
Also, to ease reading styles.ts files diffs you can hide whitespace diffs

We want the UI to adapt to normal and inverted themes

Until now we did not support Theming in the UI and we manually applied colors to component based on the expected result (normal or inverted)

In the IAP feature we will need to reuse the PromptingPage component using normal theme, but this component is implemented for the inverted theme only

So it is time to implement some Theming mechanism

The CozyTheme provider will be responsible to enforce a theme (normal or inverted) to all its children

This provider is inspired by the one on cozy-ui

Then useCozyTheme hook can be used on every component and should return the current theme name and its corresponding colors palette

This hook takes a forcedVariant parameter that can be used by top level components that are not embedded in the provider

Related files:
https://github.com/cozy/cozy-ui/blob/master/react/providers/CozyTheme/index.jsx


This PR also ports some of the following components:

  • Button
  • Typography
  • TextField

And the following views:

  • LockScreen
  • PinPrompt
  • SetPinView
  • OsReceiveScreen

Some other components still have to be adapted but this can be done in another PR. The requirement was to adapt the PromptingPage. All edits on previous components were side effects as they would have been broken when porting PromptingPage

Next to this PR, the goal would be to adapt remaining components AND to get rid of this code that should not be in the palette (its place is in colours.ts as semantic variables)


TODO:

  • Adapt Colors for MaBulle

@Ldoppea
Copy link
Member Author

Ldoppea commented Dec 11, 2023

Note that I'm not happy with the way styles has to be initialized. This code should be done in every stylized component:

  const { colors } = useCozyTheme()
  const [styles, setStyles] = useState<ButtonStyles | undefined>(undefined)

  useEffect(() => {
    setStyles(computeStyles(colors))
  }, [colors])

  if (styles === undefined) {
    return null
  }

If you know a better way to handle this?

@Ldoppea Ldoppea force-pushed the feat/cozy_theme branch 3 times, most recently from d858cf7 to f0671d1 Compare December 11, 2023 17:12
@Ldoppea
Copy link
Member Author

Ldoppea commented Dec 11, 2023

Just made this edit and this one to clean Typography usage


if (styles === undefined) {
return null
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels weird to have to handle that at the component level. We should think about an abstraction to handle that automatically (or use an existing design system)

Copy link
Member Author

@Ldoppea Ldoppea Dec 12, 2023

Choose a reason for hiding this comment

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

I changed this based on @zatteo suggestion: #1062 (comment)

Copy link
Contributor

@Crash-- Crash-- left a comment

Choose a reason for hiding this comment

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

Nice !

@zatteo
Copy link
Contributor

zatteo commented Dec 12, 2023

Note that I'm not happy with the way styles has to be initialized. This code should be done in every stylized component:

  const { colors } = useCozyTheme()
  const [styles, setStyles] = useState<ButtonStyles | undefined>(undefined)

  useEffect(() => {
    setStyles(computeStyles(colors))
  }, [colors])
  
  // styles is never undefined if I check use cozyTheme
  if (styles === undefined) {
    return null
  }

If you know a better way to handle this?

About the if (styles === undefined), styles is never undefined ? So we can remove it ?

If yes, my suggestion :

  const { colors } = useCozyTheme()
  const styles = computeStyles(colors)
  
  // Or even if we are afraid about performance
  const { colors } = useCozyTheme()
  const styles = useMemo(() => computeStyles(colors), [colors])

@Ldoppea
Copy link
Member Author

Ldoppea commented Dec 12, 2023

About the if (styles === undefined), styles is never undefined ? So we can remove it ?

If yes, my suggestion :

  const { colors } = useCozyTheme()
  const styles = computeStyles(colors)
  
  // Or even if we are afraid about performance
  const { colors } = useCozyTheme()
  const styles = useMemo(() => computeStyles(colors), [colors])

Seen during a visio: styles can be undefined because useEffect is called after the first render. But this is not the case is we use useMemo as suggested.

I made the edit here: https://github.com/cozy/cozy-flagship-app/compare/ff49720a0876d94cdf1f6a0aa56c36b1a87af1f6..05c51500c15acbd85305377d6a0f280f164acf5f

Ldoppea added a commit that referenced this pull request Dec 12, 2023
This reverts commit b11fb15

This commit collides with the CozyThem PR implementation. The easiest
way to handle conflicts is to revert this commit and to reapply the
disable condition for the SetPinView button

Related PR: #1062
@Ldoppea
Copy link
Member Author

Ldoppea commented Dec 12, 2023

I reverted @acezard PR in #1065

And I reapplied the disable logic for SetPinView's buttons here: 1046a7e

@Ldoppea Ldoppea changed the base branch from master to fix/revert_disable_style December 12, 2023 14:57
Base automatically changed from fix/revert_disable_style to master December 13, 2023 11:09
Ldoppea added a commit that referenced this pull request Dec 13, 2023
This reverts commit b11fb15

This commit collides with the CozyThem PR implementation. The easiest
way to handle conflicts is to revert this commit and to reapply the
disable condition for the SetPinView button

Related PR: #1062
We want to be able to concatenate all palette colors with Opacity
hexadecimal digits

This imply that the base color is using 6 hex digits, then when adding
Opacity digits we obtain an 8 digits hexadecimal color
We want the UI to adapt to `normal` and `inverted` themes

Until now we did not support Theming in the UI and we manually applied
colors to component based on the expected result (`normal` or
`inverted`)

In the IAP feature we will need to reuse the `PromptingPage` component
using `normal` theme, but this component is implemented for the
`inverted` theme only

So it is time to implement some Theming mechanism

The `CozyTheme` provider will be responsible to enforce a theme
(`normal` or `inverted`) to all its children

This provider is inspired by the one on cozy-ui

Then `useCozyTheme` hook can be used on every component and should
return the current theme name and its corresponding colors palette

This hook takes a `forcedVariant` parameter that can be used by top
level components that are not embedded in the provider

Related files:
https://github.com/cozy/cozy-ui/blob/master/react/providers/CozyTheme/index.jsx
Current Button implementation takes a `children` and renders it

The common Button usage is to render a `Typography` component in its
children

This implies that we know the `Typography` color to render based on the
`Button` variant (primary, disabled etc)

This behavior + the fact that there was no theming mechanism introduced
some incorrect typography colors when add the Theming mechanism

Instead of fixing every `Typography` color, we want to handle this
inside the `Button` component, so no error can be made in the future
This reapplies changes that were reverted by #1065 but without style
edits already handled by the new CozyTheme feature

Related PR: #1065
@Ldoppea Ldoppea merged commit c8ebe5a into master Dec 13, 2023
1 check passed
@Ldoppea Ldoppea deleted the feat/cozy_theme branch December 13, 2023 11:27
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.

3 participants