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

[styled-engine][styled-engine-sc] Add support for tagged template styles with tags #36403

Closed
wants to merge 3 commits into from

Conversation

htunnicliff
Copy link

@htunnicliff htunnicliff commented Mar 2, 2023

This PR implements the changes that @mnajdova shared in this comment. It should fix #28574.

This should provide the ability to use HTML tags as accessors on styled for both styled engines when writing tagged template string styles:

import { styled } from "@mui/material";

export const Example = styled.aside`
  color: red;
`;

Background

Both Styled Components and Emotion Styled docs use HTML tag accessors with tagged template strings as the default usage pattern.

Styled Components Homepage

Screenshot 2023-03-03 at 3 49 56 PM

https://styled-components.com

Emotion Styled Components Docs

Screenshot 2023-03-03 at 3 48 56 PM

https://emotion.sh/docs/styled#styling-elements-and-components

@mnajdova mnajdova added new feature New feature or request package: styled-engine Specific to @mui/styled-engine package: styled-engine-sc Specific to styled-components labels Mar 3, 2023
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this. Honestly, I am not sure if this will bring any value. The main reason for using Material UI's styled() util would be because of the additional feature it supports, which can't be provided with this syntax.

@@ -36,6 +36,10 @@ export default function styled(tag, options) {
return stylesFactory;
}

Object.keys(scStyled).forEach((tag) => {
Copy link
Member

Choose a reason for hiding this comment

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

We will need to update the type of the styled utility too, to extend ThemedStyledComponentFactories<Theme>

Copy link
Author

Choose a reason for hiding this comment

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

I'm not entirely sure which type needs to be updated, but if you let me know, I can make that change!

Copy link
Member

Choose a reason for hiding this comment

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

Similar to how the types for the @mui/styled-engine are updated. The changes would need to be made in packages/mui-styled-engine-sc/src/index.d.ts

packages/mui-styled-engine-sc/src/explore.tsx Outdated Show resolved Hide resolved
@htunnicliff
Copy link
Author

Honestly, I am not sure if this will bring any value. The main reason for using Material UI's styled() util would be because of the additional feature it supports, which can't be provided with this syntax.

Thank you for clarifying. I didn't realize this would remove features that MUI's styled() provides. Which features would be going away with this syntax?

@htunnicliff htunnicliff force-pushed the ht-add-tagged-templates branch from 5603fe5 to f1f7fa1 Compare March 3, 2023 23:17
@htunnicliff htunnicliff requested a review from mnajdova March 7, 2023 21:09
@htunnicliff
Copy link
Author

Hi @mnajdova! Is there anything else you needed from me for this PR?

@mnajdova
Copy link
Member

Thank you for clarifying. I didn't realize this would remove features that MUI's styled() provides. Which features would be going away with this syntax?

The things like specifying mui name which will support style overrides via theme, the support for sx prop on the components etc.

Hi @mnajdova! Is there anything else you needed from me for this PR?

Thanks for the mention, I will provide final feedback on Monday. I will have to think again about how we want to document this in the docs if we add it as feature. We should make it clear that it is just a duplication of emotion’s API

@mnajdova
Copy link
Member

@siriwatknp @oliviertassinari what do you think, should we proceed with adding this feature? Should we add documentation around it?

@htunnicliff
Copy link
Author

@mnajdova @siriwatknp @oliviertassinari – Checking in here. Would this PR still be a valuable feature to add?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: styled-engine Specific to @mui/styled-engine package: styled-engine-sc Specific to styled-components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[styled-engine][styled-engine-sc] Allow use of styled.div, not just styled('div')
2 participants