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

[Emotion] Convert EuiMarkdownEditor #7738

Merged
merged 17 commits into from
May 7, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented May 7, 2024

Summary

closes #6402

me: I'm going to just copy and paste some styles into a large Emotion blob. In and out, no major cleanups
also me: haha I am literally not capable of that

(in this case, some forms mixins needed to be converted from Sass to Emotion anyway, so I figured... ah what the heck, I can make do a little cleanup while here. as a treat.)

As always, please follow along by commit (or just skip to the QA portion).

QA

No visual regressions for the following UI/UX:

EuiMarkdownFormat:

EuiMarkdownEditor:

NOTE: The toolbar/footer bg colors changed slightly. I have no idea why because they're both using lightestShade. I think Caroline changed this in the Sass->Emotion work.

Checklist

See the Emotion conversion checklist in #6402

cee-chen added 14 commits May 6, 2024 13:23
- remove unnecessary `margin-bottom !important` - doesn't appear to be doing anything whatsoever
+ export new/internal `isNamedColor` util from EuiTextColor
- move them from global CSS to the specific plugin/renderer

+ DRY out readonly CSS by moving it to the default checkbox component styles, and setting the actual inputs to `readonly` (which is significantly more accessible to screen readers)
+ merge amsterdam overrides

+ merge preview styles, doesn't have a separate component

+ start component variables fn
- skip param args in favor of CSS variables (much cleaner all around)

- these utils may need some more cleanup as I move into the actual forms conversions, but they're probably Good Enough for now
- look at that CSS variable in action! wowowow

note: I'm skipping reducing nesting specificity and !importants for this component because its ownership is kind of up in the air and it's probably not worth the time right now
- not sure why this wasn't already the case in prod, so making this opinionated change now
+ replace an unnecessary snapshot with an assertion
@cee-chen cee-chen changed the title [Emotion] Convert EuiMarkdownEditor [Emotion] Convert EuiMarkdownEditor May 7, 2024
@cee-chen cee-chen marked this pull request as ready for review May 7, 2024 04:45
@cee-chen cee-chen requested a review from a team as a code owner May 7, 2024 04:45
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Copy link
Contributor

@mgadewoll mgadewoll 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 the fixes and clarifying the comments! It works nicely now 🎉

@cee-chen
Copy link
Member Author

cee-chen commented May 7, 2024

Thanks as always for the amazing QA Lene, you rock!

@cee-chen cee-chen merged commit d915ac1 into elastic:main May 7, 2024
5 checks passed
@cee-chen cee-chen deleted the emotion/markdown-editor branch May 7, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Emotion] EuiMarkdownEditor
4 participants