Skip to content

[Avatar] Use styled-components#21104

Closed
eps1lon wants to merge 2 commits into
mui:masterfrom
eps1lon:feat/styled-components
Closed

[Avatar] Use styled-components#21104
eps1lon wants to merge 2 commits into
mui:masterfrom
eps1lon:feat/styled-components

Conversation

@eps1lon

@eps1lon eps1lon commented May 18, 2020

Copy link
Copy Markdown
Member

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 Introduces changes that are not backward compatible. scope: avatar Changes related to the avatar. 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

NMinhNguyen commented May 20, 2020

Copy link
Copy Markdown
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

eps1lon commented May 20, 2020

Copy link
Copy Markdown
Member Author

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

NMinhNguyen commented May 20, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

eps1lon commented May 20, 2020

Copy link
Copy Markdown
Member Author

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

NMinhNguyen commented May 20, 2020

Copy link
Copy Markdown
Contributor

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

merceyz
merceyz reviewed May 31, 2020
View reviewed changes
Comment thread packages/material-ui/src/Avatar/Avatar.js
import Person from '../internal/svg-icons/Person';

export const styles = (theme) => ({
export const styles = {

@merceyz merceyz May 31, 2020

Copy link
Copy Markdown
Collaborator

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
@oliviertassinari oliviertassinari mentioned this pull request Jun 15, 2020
Closed
@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

Reviewers

@oliviertassinari oliviertassinari Awaiting requested review from oliviertassinari
1 more reviewer
@merceyz merceyz merceyz left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

breaking change Introduces changes that are not backward compatible. PR: out-of-date The pull request has merge conflicts and can't be merged. scope: avatar Changes related to the avatar.

Projects

None yet

Milestone

v5.0.0-beta.0

Development

Successfully merging this pull request may close these issues.

3 participants

@eps1lon @NMinhNguyen @merceyz