[Emotion] Add internal service for cloning elements with the css property
#5835
Conversation
|
I have this working for EuiTitle in my local (the component I was originally trying to convert to Emotion):
In this case the only CSS I've added so far was |
| * | ||
| * NOTE: We're still using/testing this utility internally, so this is not yet a public API | ||
| */ | ||
| export const cloneElementWithCss = ( |
There was a problem hiding this comment.
Not super in love with this name or anything, would be open to other suggestions/preferences, e.g. emotionClone
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5835/ |
| if (props.css || element.props.css) { | ||
| clonedProps.css = [element.props.css, props.css]; | ||
| } |
There was a problem hiding this comment.
this doesn't seem to be necessary from my unit testing, css is merging everything on its own. should I remove it?
There was a problem hiding this comment.
This code does have an impact based on my testing.
Given
<EuiScreenReaderOnly
css={css`
color: coral;
`}
>
<p
css={css`
background-color: gainsboro;
`}
>
This is the second paragraph. It is hidden for sighted users but visible
to screen readers.
</p>
</EuiScreenReaderOnly>Without this code, background-color: gainsboro; is not applied.
With this code, background-color: gainsboro; is applied first in the style order. (style order: p inherent EuiScreenReaderOnly emotion styles, EuiScreenReaderOnly css prop styles)
There was a problem hiding this comment.
Super baffling - I can't seem to reproduce this behavior in the unit tests in this PR. I'm not exactly sure what the difference is between EuiScreenReaderOnly and Jest/JSDOM. If it works for you though no worries, let's leave it in!
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5835/ |
|
No rush at all as I'm still sorting out my git history, but my EuiTitle work will need this service as a heads up! |
Is there any update on this? I'm very interested in this as the workaround in #1404 (by using |
|
Thanks for checking in on this @LukasKalbertodt! It's always great to hear when something will be useful to consumers. We've been using this utility fairly extensively internally for a while now with no apparent bugs, and I personally think this service is stable enough to make a top-level EUI export. I'll open a PR for that here shortly. |
문제 상황 - React.cloneElement를 사용하여 자식 컴포넌트를 복제한 뒤 css props를 전달하면 css가 적용이 안되는 에러 발생 문제 원인 - ReactElement는 css prop을 가지고 있지 않음 - css props는 이모션이 제공하는 커스텀 props로, 이모션은 css props를 받으면 자체 렌더링하여 스타일을 입힘 - React.cloneElement시 css를 props로 전달하면 일반 props로 인식되고, 스타일링이 입혀지지 않음 (elastic/eui#5835) 해결 방안 - css props가 존재할 경우 이모션에서 제공하는 jsx() 함수를 사용하여 이모션이 렌더링하는 방식으로 구현함 평가 - 이모션에서는 cloneElementWithCss와 같은 내장 함수를 추가하여 v11 버전을 출시할 계획이 있다고 밝힘
Summary
React.cloneElementdoes not work when settingcssif the cloned element does not already have thecssprop. To support cloningcss, we need to create an internal helper that usesjsx()when cloning elements in the above scenario.See:
jsx(…)fails when “cloning” element withcssprop emotion-js/emotion#1404Per note from @thompsongl, we're holding off on exporting this as a public API until we've tested it more thoroughly internally.
Checklist
- [ ] Checked in both light and dark modes- [ ] Checked in mobile- [ ] Checked in Chrome, Safari, Edge, and Firefox- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Updated the Figma library counterpartOnly needed if public export:
- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] A changelog entry exists and is marked appropriately