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] Allow use of styled.div, not just styled('div') #28574

Open
1 task done
markalfred opened this issue Sep 24, 2021 · 18 comments
Open
1 task done
Labels
new feature New feature or request package: styled-engine Specific to @mui/styled-engine package: styled-engine-sc Specific to styled-components ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@markalfred
Copy link

  • I have searched the issues of this repository and believe that this is not a duplicate.

The only other issue regarding this is #22147.

Summary 💡

When using Emotion's styled, it's possible to use the syntax:

const Flex = styled.div`
  display: flex;
`

This doesn't work when importing styled from @mui/materials/styles. Instead, this is required:

const Flex = styled('div')`
  display: flex;
`

This is fine, but Emotion's syntax is a little nicer.

Examples 🌈

Note Emotion's styled documentation examples:
image

Motivation 🔦

As a developer who's used to Emotion, I've found myself making this mistake repeatedly over the past few days. It'd be nice for MUI to have the same syntactic sugars that Emotion has.

Relevant Code

This is the code on Emotion's side that powers this feature:

https://github.com/emotion-js/emotion/blob/main/packages/styled/src/index.js

// @flow
import styled from './base'
import { tags } from './tags'

// bind it to avoid mutating the original function
const newStyled = styled.bind()

tags.forEach(tagName => {
  // $FlowFixMe: we can ignore this because its exposed type is defined by the CreateStyled type
  newStyled[tagName] = newStyled(tagName)
})

export default newStyled

Thanks for all your hard work and efforts, v5 is looking amazing.

@markalfred markalfred added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 24, 2021
@HyphenFrox
Copy link

HyphenFrox commented Sep 24, 2021

@markalfred I am pretty sure you can you use emotion's styled.div syntax. Just make sure you have @mui/material @emotion/react and @emotion/styled installed. Please run one of the following commands:-

// with npm
npm install @mui/material @emotion/react @emotion/styled

// with yarn
yarn add @mui/material @emotion/react @emotion/styled

And then import styled from "@emotion/styled"

check this code sandbox https://codesandbox.io/s/modern-wildflower-hijbc?file=/src/App.js

@markalfred
Copy link
Author

@HyphenFrox you're totally right. I didn't think theme would be available unless I imported from @mui/material/styles but I see it there in your codesandbox. Thanks for the response!

@HyphenFrox
Copy link

HyphenFrox commented Sep 24, 2021

Please don't import anything from @mui/material/styles unless absolutely necessary. I believe they are legacy imports. That's why they didn't have the functionality you were looking for. Try to import everything from @mui/system or @mui/material

You can use the theme in the styled function as demonstrated at https://mui.com/system/styled/#using-the-theme.

Don't forget to close the issue.

@markalfred
Copy link
Author

Got it. Just want to note that this results in an error:

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

const MyDiv = styled.div`
  background-color: red;
`

// => TypeError: _system.styled.div is not a function

I'll use @emotion/styled from now on, thanks for the help.

@mbrookes mbrookes changed the title Allow usage of styled.div, not just styled('div') Allow use of styled.div, not just styled('div') Sep 24, 2021
@kdgit1
Copy link

kdgit1 commented Sep 25, 2021

How do I use @emotion style syntax when passing theme ?

This works:
import { styled, createTheme, ThemeProvider } from '@mui/system';

const MyTestButton = styled(Button)(({ theme }) => ({
color: theme.palette.mode === 'dark' ? '#fff' : '#000',
padding: '20px 10px',
borderRadius: 0,
fontWeight: 500,
fontSize: '0.95em',
'&:hover, &:focus': {
backgroundColor: theme.palette.mode === 'dark' ? '#eee' : '#222',
},
}));

I would really prefer using standard css inside the style block ( the emotion syntax / kebab-case) vs. using the camelCase and values in single quotes.

I did try enclosing the function within template literals but that does not seem to work either when passing the theme. Any pointers or a simple example would be appreciated.

Thanks

@HyphenFrox
Copy link

HyphenFrox commented Sep 25, 2021

@kdgit1 If you want to use the styled.div syntax you have to import styled from @emotion/styled and from that point you are using emotion, not MUI so you have to follow emotion's guide on using the theme. https://emotion.sh/docs/theming#styled . Emotion's guide says that the Theme object is available as props.theme

@kdgit1
Copy link

kdgit1 commented Sep 25, 2021

Thanks for your quick response.
I was able to make it work by wrapping the app with ThemeProvider from emotion and passing the same theme object that is passed into the Mui ThemeProvider

Works like a charm ( for now; haven't tested out everything but the basics sure seem to work just fine ). So glad to be able to write css as simple clean css.

@HyphenFrox
Copy link

@kdgit1 You don't need to import ThemeProvider from emotion. Just use the ThemeProvider from @mui/material. Emotion will still read your theme object from MUI. I updated the codesandbox if you want an example. https://codesandbox.io/s/modern-wildflower-hijbc?file=/src/App.js

@markalfred
Copy link
Author

I'd like to re-open this for discussion as a new wrinkle has come up for my team. While importing from @emotion/styled gets us the nicer styled.div syntax, it breaks TypeScript support for the theme prop. So my previous solution of using @emotion/styled doesn't work, and we're forced to import from @mui/system/styled instead. This breaks the styled.div syntax and instead we're forced to use styled('div').

image

https://codesandbox.io/s/using-styled-div-in-mui-with-theme-forked-86gol?file=/src/App.tsx

@markalfred markalfred reopened this Oct 8, 2021
@mnajdova
Copy link
Member

mnajdova commented Oct 8, 2021

I think we should add the primitives. This would be a good starting point:

index ad0e2c17f9..e5e877e655 100644
--- a/packages/mui-styled-engine-sc/src/index.js
+++ b/packages/mui-styled-engine-sc/src/index.js
@@ -34,6 +34,10 @@ export default function styled(tag, options) {
   return (...styles) => stylesFactory(...styles);
 }

+Object.keys(scStyled).forEach((tag) => {
+  styled[tag] = scStyled[tag];
+});
+
 export { ThemeContext, keyframes, css } from 'styled-components';
 export { default as StyledEngineProvider } from './StyledEngineProvider';
 export { default as GlobalStyles } from './GlobalStyles';
diff --git a/packages/mui-styled-engine-sc/src/styled.test.js b/packages/mui-styled-engine-sc/src/styled.test.js
index 0b1f7957c1..de1d675664 100644
--- a/packages/mui-styled-engine-sc/src/styled.test.js
+++ b/packages/mui-styled-engine-sc/src/styled.test.js
@@ -1,5 +1,6 @@
 import * as React from 'react';
 import { expect } from 'chai';
+import scStyled from 'styled-components';
 import { createClientRender } from 'test/utils';
 import styled from '@mui/styled-engine-sc';

@@ -38,4 +39,8 @@ describe('styled', () => {
   it("should not allow styled-components's APIs: .attrs", () => {
     expect(typeof styled('span').attrs).to.equal('undefined');
   });
+
+  it('has primitive', () => {
+    expect(styled.div).to.equal(scStyled.div);
+  });
 });
diff --git a/packages/mui-styled-engine/src/index.js b/packages/mui-styled-engine/src/index.js
index 696af8f9b8..8074e6395b 100644
--- a/packages/mui-styled-engine/src/index.js
+++ b/packages/mui-styled-engine/src/index.js
@@ -25,6 +25,10 @@ export default function styled(tag, options) {
   return stylesFactory;
 }

+Object.keys(emStyled).forEach((tag) => {
+  styled[tag] = emStyled[tag];
+});
+
 export { ThemeContext, keyframes, css } from '@emotion/react';
 export { default as StyledEngineProvider } from './StyledEngineProvider';
 export { default as GlobalStyles } from './GlobalStyles';
diff --git a/packages/mui-styled-engine/src/styled.test.js b/packages/mui-styled-engine/src/styled.test.js
index d7e51ee936..2fdbbcdbc3 100644
--- a/packages/mui-styled-engine/src/styled.test.js
+++ b/packages/mui-styled-engine/src/styled.test.js
@@ -1,4 +1,5 @@
 import { expect } from 'chai';
+import emStyled from '@emotion/styled';
 import styled from './index';

 describe('styled', () => {
@@ -11,4 +12,8 @@ describe('styled', () => {
       styled('span')(undefined, { color: 'red' });
     }).toErrorDev('MUI: the styled("span")(...args) API requires all its args to be defined');
   });
+
+  it('has primitive', () => {
+    expect(styled.div).to.equal(emStyled.div);
+  });
 });

I can help if someone wants to take this up.

@markalfred if you want to use direction emotion's styled() utility you need to follow https://emotion.sh/docs/typescript#define-a-theme for defining the typings for the Theme.

@kdgit1 you can use the styled() from @mui/material/styles with the CSS syntax. What was not working for you?

@mnajdova mnajdova added new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted package: system Specific to @mui/system and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 8, 2021
@mnajdova mnajdova changed the title Allow use of styled.div, not just styled('div') [styled-engine][styled-engine-sc] Allow use of styled.div, not just styled('div') Oct 8, 2021
@mnajdova mnajdova added package: styled-engine Specific to @mui/styled-engine package: styled-engine-sc Specific to styled-components and removed package: system Specific to @mui/system labels Oct 8, 2021
@dalechyn
Copy link

dalechyn commented Feb 1, 2022

@mnajdova has been any progress on this?
Docs say that styled-engine-sc is a wrapper, but in reality, it cuts off functionality described in this issue as well as #29723 (which looks like a duplicate), as well as it cuts off .attrs functionality as mentioned in #29492.
If so, styled-engine-sc should not be stated as a wrapper, but rather as a fork with intersectioned with @emotion/styled interface.

@mnajdova
Copy link
Member

mnajdova commented Feb 1, 2022

@h0tw4t3r I don't see why we shouldn't add the API proposed in my previous comment, feel free to grab it you want to work on it. styled-engine-sc is an adapter, not wrapper, agree we should clarify that. Could you please link where did you read that it is a wrapper?

@dalechyn
Copy link

dalechyn commented Feb 1, 2022

@mnajdova Sure.
https://mui.com/guides/styled-engine/#how-to-switch-to-styled-components

  • @mui/styled-engine - a thin wrapper around emotion's styled() API, with the addition of few other required utilities, such as the component, the css and keyframe helpers, etc. This is the default.
  • @mui/styled-engine-sc - a similar wrapper around styled-components.

styled.div, styled.small, etc. are very useful shorthands for styled('div'), styled('small').

Those who used styled-components will probably agree that styled(...) notation is used often for React components and not default HTML tags.

I'm not familiar with @emotion/styled, but I personally don't see a reason to cut off such functionality if such exists only for upcoming developers switching to @mui to ease their migration.

Can you please tell me if the patch provided above just misses the TypeScript part of it?
I could make a PR if so.

@mnajdova
Copy link
Member

mnajdova commented Mar 3, 2022

@h0tw4t3r sorry for the delay, let's add the patch and some spec tests and we can see what's missing. Are you still interested to work on it?

@dalechyn
Copy link

dalechyn commented Mar 6, 2022

@h0tw4t3r sorry for the delay, let's add the patch and some spec tests and we can see what's missing. Are you still interested to work on it?

I'll work in the free time, yes.

@mnajdova
Copy link
Member

mnajdova commented Mar 7, 2022

Perfect, will look out for the PR :)

@geraldaddey
Copy link

mnajdova

Would like to make this PR. Can you guide me?

@mnajdova
Copy link
Member

@geraldaddey sorry for the late response, the guidance was provided in a comment above - #28574 (comment)

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 ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
6 participants