[Emotion] Convert EuiLink#5856
Conversation
|
@thompsongl Something odd is happening on CI when it's trying to create TS defs on this PR for EuiLink: I couldn't find anything specifically that would have affected this in the changeset. Can you take a look? |
elizabetdev
left a comment
There was a problem hiding this comment.
Thanks, @MichaelMarcialis for doing this conversion.
There are some things that can be improved and I added some suggestions.
I'm just not completely sure about the best way of doing the mixins. @constancecchen or @cchaos can take a look at the mixins to see if they can be improved?
I have an idea, but everything passes locally. So I'm just going to push commits here until it's resolved. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5856/ |
6be8eea resolved the CI failure |
…sion # Conflicts: # src/global_styling/mixins/index.ts
|
Thanks so much for the detailed review, @miukimiu! Please forgive all my rookie mistakes 😬 I believe I've addressed all your comments, but let me know if I've missed anything. While making these changes, two questions came to mind regarding modifier styles:
Thanks again! |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5856/ |
cchaos
left a comment
There was a problem hiding this comment.
Thanks @MichaelMarcialis! I found just a couple other things and some nits.
Regarding BEM naming...
As you can see from the snapshot out of the className we don't have to manually apply BEM naming for modifiers:
Which is why we always make sure the name of the component is the first style key, in this case euiLink. Then Emotion will apply the subsequent style keys as -modifier to the className.
css-uj6g16-euiLink-button-colorStyles-accent
The same applies for children. The first key we grab and pass to the children needs to be the euiComponentName__childName so that it's output first as you can see in the external icon use:
class="css-jqzood-euiLink__externalIcon"
src/components/link/link.styles.ts
Outdated
| ${colorStyles(euiTheme.colors.primaryText)} | ||
| `, | ||
| subdued: css` | ||
| ${colorStyles(euiTheme.colors.subdued)} |
There was a problem hiding this comment.
Not to self, I think this token name should change to subduedText to be explicit.
|
Thanks, @cchaos! I've updated the PR with your feedback. Let me know if I missed anything. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5856/ |
cchaos
left a comment
There was a problem hiding this comment.
🙌 LGTM! Just one last nit and maybe a conflict resolve gone bad? Otherwise, should be good to merge
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5856/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5856/ |
Summary
This PR converts the majority of the EuiLink component to CSS-in-JS. The only outstanding SCSS is the portion that references the
euiFocusRingmixin, which has yet to be converted to Emotion. Once that has been converted, we can go back and remove that last bit of SCSS.TODOcomments have been added to echo that statement in the appropriate files.I did not add a deprecation warning to the global and Amsterdam
euiLinkmixins, as it is being used by other yet-to-be-converted components in EUI. However, if the deprecation warning should be added regardless, please let me know.Big thanks to @cchaos for the guidance!
Things to look out for when moving styles
[ ] Anything in the backlog that could be a quick fix for that component?[ ] Convert global Sass vars to JS version like sizing[ ] Convert component-specific Sass vars to exported JS versions[ ] Convert side specific padding, margin, and position to-inlineand-block(Logical properties)[ ] Usegapproperty to add margin between items if using flex[ ] Can any still existing.jsfiles be converted to TS?[ ] All animations or transitions are wrapped ineuiCanAnimateQA