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

Fix useTheme hook #923

Closed
4 tasks
NicholasBoll opened this issue Dec 4, 2020 · 0 comments · Fixed by #2120
Closed
4 tasks

Fix useTheme hook #923

NicholasBoll opened this issue Dec 4, 2020 · 0 comments · Fixed by #2120
Assignees
Labels
9.x breaking-change A change that will break something for consumers bug Something isn't working
Milestone

Comments

@NicholasBoll
Copy link
Member

NicholasBoll commented Dec 4, 2020

🐛 Bug Report

The useTheme hook is used in 3 distinct ways:

  • Inside a functional component
  • Inside a styled function
  • Inside a class component

All use-cases will use the getFilledTheme utility function or fall back to the default Canvas theme if no theme was found.

There is actually 2 distinct functions: A hook that can access the ThemeContext and a utility function. At the moment, the way it is implemented shouldn't cause any real issues, but every use of the CK styled function that doesn't have a predefined theme is throwing a silently-ignored error which isn't very efficient.

I propose useTheme is broken out to 2 distinct functions:

function isCanvasTheme(input: object): input is EmotionCanvasTheme {
  return input.hasOwnProperty('canvas');
}

function getThemeFromWindow(): any {
  // should we do any validation here? The shape could be anything... We're returning `any` and pretending things will be okay
  return typeof window !== 'undefined' && get(window, 'window.workday.canvas.theme');
}

// to be used in `styled` and class components
export function getTheme(theme?: PartialEmotionCanvasTheme): EmotionCanvasTheme {
  if (theme && isCanvasTheme(theme)) {
    return getFilledTheme(theme);
  }

  const windowTheme = getThemeFromWindow();
  if (windowTheme) {
    return getFilledTheme({canvas: windowTheme});
  }

  return {canvas: defaultCanvasTheme};
}

// to be used in functional components
export function useTheme(theme?: PartialEmotionCanvasTheme): EmotionCanvasTheme {
  const contextTheme = React.useContext(ThemeContext);

  if (theme && isCanvasTheme(theme)) {
    return getFilledTheme(theme);
  }

  if (isCanvasTheme(contextTheme)) {
    return getFilledTheme(contextTheme);
  }

  const windowTheme = getThemeFromWindow();
  if (windowTheme) {
    return getFilledTheme({canvas: windowTheme});
  }

  return {canvas: defaultCanvasTheme};
}

AC

  • Update components that current useTheme to use the new methods
  • Deprecate old useTheme methods
  • Document changes
  • Review current usage & plan for migration

look into #864 and if that can be somewhat addressed here

@NicholasBoll NicholasBoll added the bug Something isn't working label Dec 4, 2020
@jpante jpante added the p:2 label Dec 7, 2020
@jpante jpante added 5.x breaking-change A change that will break something for consumers labels Dec 7, 2020
@jpante jpante added 6.x and removed 5.x labels Apr 13, 2021
@alanbsmith alanbsmith added this to the 6.0.0 milestone Sep 8, 2021
@myvuuu myvuuu removed the 6.x label Sep 30, 2021
@alanbsmith alanbsmith removed this from the 6.0.0 milestone Oct 22, 2021
@jaclynjessup jaclynjessup removed the p:2 label Aug 5, 2022
@jaclynjessup jaclynjessup added this to the 9.0.0 milestone Nov 9, 2022
@jaclynjessup jaclynjessup changed the title useTheme is not a real hook Fix useTheme hook Jan 12, 2023
@alanbsmith alanbsmith added the 9.x label Mar 17, 2023
@RayRedGoose RayRedGoose self-assigned this Mar 21, 2023
@RayRedGoose RayRedGoose linked a pull request Apr 3, 2023 that will close this issue
10 tasks
alanbsmith pushed a commit that referenced this issue Apr 7, 2023
Fixes: #923 

- We've added `getTheme` so that you can have access to theme inside of `styled` function or class components.
- We've updated `useTheme` to add a warning if being used outside of a context. 
- We've removed `getCanvasTheme` and `useCanvasTheme`.

[category:Components]

### BREAKING CHANGES
- We've removed `getCanvasTheme` and `useCanvasTheme`.
- Use `useTheme` when you have a `CanvasProvider` or you're inside of a functional component
- Use `getTheme` when inside a styled component and you need access to theme.

Co-authored-by: @mannycarrera4 <[email protected]>
@github-actions github-actions bot closed this as completed Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9.x breaking-change A change that will break something for consumers bug Something isn't working
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

6 participants