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

Enhance useTheme to provide default theme without ThemeProvider #105

Merged
merged 4 commits into from
Sep 10, 2021
Merged

Enhance useTheme to provide default theme without ThemeProvider #105

merged 4 commits into from
Sep 10, 2021

Conversation

yujonglee
Copy link
Contributor

@yujonglee yujonglee commented Sep 8, 2021

Description

In Short: Providing default theme using withTheme, default value, etc... often fails. This PR solve this problem by enhancing useTheme in dooboo-ui. Now, useTheme will at least return default theme(light) even if there aren't ThemeProvider exists.


Providing theme is complicated task. And providing default theme is harder.

dooboo-ui aims to provide default theme even if user forget to wrap componets with ThemeProvider.

Currently, theme providing is done by this.

const ButtonComponent: FC<ButtonProps & {theme: DoobooTheme}> = ({})

export const Button = withTheme(ButtonComponent);

and providing default theme(when ThemeProvider is not provided) is done by this.

// Sol 1
ButtonComponent.defaultProps = {theme: defaultTheme};

// Sol 2
const {theme = defaultTheme} = props

However, default theme is not applied as I mentioned in #104.

It is because withTheme from emotion provides { } when there aren't any theme to provide. So props.theme is never undefined, resulting our defaultTheme never applied. See this line of emotion implementation.

So, I made a workaround in #86 using isEmptyObject.


But it is just a workaround. It should be much easier and effortless to do it.
So I re-write useTheme and provide light to be default theme.

Now, there's no need to have theme prop, withTheme HOC, and default value. All you need to do to get current theme is calling useTheme. This will reduce confusion a lot.

In this PR, I refactor only SwitchToggle using new useTheme.

Test Plan

none.

Related Issues

#104

Tests

none.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Run yarn test or yarn test -u if you need to update snapshot.
  • Run yarn lint
  • I am willing to follow-up on review comments in a timely manner.

@yujonglee
Copy link
Contributor Author

I'll solve build error and open it again.

@yujonglee yujonglee marked this pull request as draft September 8, 2021 09:18
import styled from '@emotion/native';
import {withTheme} from '@emotion/react';
import {useTheme} from './theme/ThemeProvider';

interface Styles {
containerStyle?: ViewStyle;
Copy link
Member

Choose a reason for hiding this comment

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

You may remove prefix Style when it is grouped under Styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I'll remove it in other PR. (because it is not related to this PR.)

@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #105 (c9f1cc7) into master (0d8318d) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
- Coverage   95.53%   95.50%   -0.04%     
==========================================
  Files          23       22       -1     
  Lines         582      578       -4     
  Branches      267      265       -2     
==========================================
- Hits          556      552       -4     
  Misses         26       26              

@yujonglee yujonglee marked this pull request as ready for review September 8, 2021 17:18
@yujonglee yujonglee marked this pull request as draft September 9, 2021 08:11
@yujonglee yujonglee marked this pull request as ready for review September 9, 2021 11:38
@yujonglee yujonglee changed the title Make useTheme provide default theme without ThemeProvider Enhance useTheme to provide default theme without ThemeProvider Sep 10, 2021
Comment on lines +10 to +13
function createCtx<A>(defaultContext: A): CreateCtx<A> {
const ctx = React.createContext<A>(defaultContext);

function useCtx(): A {
const c = React.useContext(ctx);

if (!c) throw new Error('useCtx must be inside a Provider with a value');

return c;
}
const useCtx = (): A => React.useContext(ctx);
Copy link
Member

Choose a reason for hiding this comment

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

This is very clever~! However, I am worried that many devs will be curious that useTheme works without a Provider because usually context wasn't provided like that 🤔

I hope there is more thing related to this to read about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hyochan Ok. I will write about it soon.

Copy link
Member

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

Although I am curious about providing default context value, I think this PR is still great to have.

@hyochan hyochan merged commit b69c16d into dooboolab-community:master Sep 10, 2021
@yujonglee
Copy link
Contributor Author

yujonglee commented Sep 10, 2021

@hyochan Maybe this helps about providing default context value.

<Provider
  value={{
    media,
    themeType,
    changeThemeType,
    theme,
    colors,
  }}>
  <OriginalThemeProvider theme={{...theme, ...media}}>
    {children}
  </OriginalThemeProvider>
</Provider>

It will be best if we can give default context value to OriginalThemeProvider(by emotion), but currently there's no way to do that.(As I know). So we can just give default context value to our own Provider.

Therefore, default theme can be only accessed with our own useTheme, not useTheme, withTheme, props in styled by emotion.

@yujonglee yujonglee mentioned this pull request Sep 11, 2021
4 tasks
@yujonglee yujonglee added this to In progress in dooboo-ui/theme Sep 23, 2021
@yujonglee yujonglee moved this from In progress to Review in progress in dooboo-ui/theme Sep 23, 2021
@yujonglee yujonglee moved this from Review in progress to Done in dooboo-ui/theme Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants