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

[Avatar] Use styled-components #21104

Closed
wants to merge 2 commits into from

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented May 18, 2020

Quick prototype on how to migrate to styled-components (definitely need a codemod).

  • styled-components has a single theme context. We cannot use it to avoid breaking changes with a possible existing styled-components theme and because we couldn't use a default theme
  • interop with theme.props (implemented) and theme.overrides (not implemented yet) might be awkward
  • How to get displayName to work? styled.div is not helpful
  • with this approach we can keep the classes API

@eps1lon eps1lon added breaking change component: avatar This is the name of the generic UI component, not the React module! labels May 18, 2020
@eps1lon eps1lon added this to the v5 milestone May 18, 2020
@eps1lon eps1lon requested a review from oliviertassinari May 18, 2020 17:35
@NMinhNguyen
Copy link
Contributor

styled-components has a single theme context.

Just for my own understanding, how many does JSS have? It’s 1 as well, right?

@eps1lon
Copy link
Member Author

eps1lon commented May 20, 2020

Just for my own understanding, how many does JSS have? It’s 1 as well, right?

You can create as many as you want with createTheming.

I'm pretty sure we can't (no default theme) and shouldn't (possible collision with existing theme) use the theming of styled-components.

@NMinhNguyen
Copy link
Contributor

NMinhNguyen commented May 20, 2020

Oh right, but we’re also not currently using it but defining our own context? Didn’t find references to createTheming in the codebase.

@eps1lon
Copy link
Member Author

eps1lon commented May 20, 2020

We have our own bindings to jss in @material-ui/styles and therefore no need to createTheming. If we'd use react-jss we'd use createTheming

@NMinhNguyen
Copy link
Contributor

Cool, was just trying to compare the two approaches because that point stood out to me.

import Person from '../internal/svg-icons/Person';

export const styles = (theme) => ({
export const styles = {
Copy link
Member

Choose a reason for hiding this comment

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

For future reference: Should be possible to move this into the typescript definitions and get rid of the size cost + get documentation on the classes in the editor. Would need to update the documentation generation to read from the proptypes instead though.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 3, 2020
@eps1lon eps1lon closed this Jul 7, 2020
@eps1lon eps1lon deleted the feat/styled-components branch July 7, 2020 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: avatar This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants