[Emotion] Convert EuiCallOut by cchaos · Pull Request #5870 · elastic/eui

cchaos · 2022-05-04T23:07:41Z

Summary

Converts EuiCallOut to remove custom styles but instead directly use EuiPanel as the parent component, and EuiTitle for the heading. I'll make in-code comments about the rest of the changes.

Also fixes callout portion of #5111 by moving the icon inline with the heading text and just letting the text wrap:

Also increased the default contrast ratio to `4.85`

This is a very slight change mainly to fix our primary text which was not getting calculated high enough to be able to display on top of the primary panel background:

So instead of trying to do specific calculations for callout, I opted for just increasing the minimum contrast for all text calculations since we'll likely see more and more uses of theses shaded backgrounds. The visible changes are very minimal and have most effect on the primary color:

The good news is that now we can be pretty certain that if anyone uses these background colors and the text colors together, they'll pretty much all work.

Checklist

Checked in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
[ ] Props have proper autodocs and playground toggles
Updated documentation
~~[ ] Checked Code Sandbox works for any docs examples~~
Added or updated jest and cypress tests
~~[ ] Checked for breaking changes and labeled appropriately~~
Checked for accessibility including keyboard-only and screenreader modes
Updated the Figma library counterpart
A changelog entry exists and is marked appropriately

Things to look out for when moving styles

Anything in the backlog that could be a quick fix for that component?
Convert side specific padding, margin, and position to -inline and -block (Logical properties)
Reduce specificity where possible (usually by reducing class name chaining)
Can any still existing .js files be converted to TS?

QA

Does the output CSS match the previous CSS / as expected
Does the rendered class read as expected

cchaos · 2022-05-04T23:09:06Z

src/components/call_out/_mixins.scss

+// Kibana's Ace Editor uses this function, can't remove yet
+// https://github.com/elastic/kibana/blob/main/src/core/public/styles/_ace_overrides.scss
+// As well as deprecation notices
+// https://github.com/elastic/kibana/blob/main/x-pack/plugins/upgrade_assistant/public/application/components/es_deprecations/deprecation_types/reindex/flyout/_step_progress.scss
 @function euiCallOutColor($type: 'primary', $returnBackgroundOrForeground: 'background') {


I don't know if we want to leave comments specifically pointing to Kibana files, but figured since I found them anyway?

cchaos · 2022-05-04T23:09:45Z

src/components/call_out/call_out.styles.ts

+  return {
+    euiCallOutHeader: css`
+      font-weight: ${euiTheme.font.weight.medium};
+      margin-bottom: 0 !important; // In case it's nested inside EuiText


Specificity shot me on this one

Sometimes an !important tag is what's needed.

cchaos · 2022-05-04T23:10:40Z

src/components/call_out/call_out.tsx

+
    const classes = classNames(
      'euiCallOut',
      colorToClassNameMap[color],


Unfortunately I couldn't get rid of the color class name because a lot of tests were specifically pointing to these

cchaos · 2022-05-04T23:11:42Z

src/components/call_out/call_out.tsx

    }

-    const H: any = heading ? `${heading}` : 'span';
+    const H: any = heading ? `${heading}` : 'p';


I changed the default heading here to p because it felt odd to not have text in something text-like especially if it's then followed by more text.

Instead of doing this, you can just pick heading = 'p', as the default prop. The same way we're doing with color = 'primary':

eui/src/components/call_out/call_out.tsx

Lines 46 to 57 in f209dd4

export const EuiCallOut = forwardRef<HTMLDivElement, EuiCallOutProps>(

(

{

title,

color = 'primary',

size = 'm',

iconType,

children,

className,

heading,

...rest

},

Done in 8cde1f1

cchaos · 2022-05-04T23:12:06Z

src/components/call_out/call_out.tsx

-          <H className="euiCallOutHeader__title">{title}</H>
-        </div>
+        <EuiTitle size={size === 's' ? 'xxs' : 'xs'} css={cssHeaderStyles}>
+          <H className="euiCallOutHeader__title">


This className too is targeted a lot in tests ☹️

cchaos · 2022-05-04T23:12:22Z

src/components/call_out/call_out.tsx

+        // @ts-expect-error Help with forward ref
+        ref={ref}


Nightmaref! 😆

😦 @thompsongl Maybe you can help?

Fixed in 6a85444

EuiPanel takes panelRef instead of ref

🤦‍♀️ Oh boy... TY!!!

kibanamachine · 2022-05-04T23:48:06Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5870/

elizabetdev

Thanks, @cchaos. The icon inline with the heading text looks much better! 😍

Tested in Chrome, Safari, Edge, and Firefox. LGTM! 🎉

elizabetdev · 2022-05-05T15:34:16Z

src/components/call_out/call_out.tsx

    }

-    const H: any = heading ? `${heading}` : 'span';
+    const H: any = heading ? `${heading}` : 'p';


Instead of doing this, you can just pick heading = 'p', as the default prop. The same way we're doing with color = 'primary':

eui/src/components/call_out/call_out.tsx

Lines 46 to 57 in f209dd4

export const EuiCallOut = forwardRef<HTMLDivElement, EuiCallOutProps>(

(

{

title,

color = 'primary',

size = 'm',

iconType,

children,

className,

heading,

...rest

},

elizabetdev · 2022-05-05T15:35:19Z

src/components/call_out/call_out.tsx

+        // @ts-expect-error Help with forward ref
+        ref={ref}


Nightmaref! 😆

kibanamachine · 2022-05-05T19:22:09Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5870/

cchaos · 2022-05-05T19:35:55Z

Flakey?

Jenkins, test this

kibanamachine · 2022-05-05T20:03:43Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5870/

1Copenut

LGTM! One suggested change with a case for why, but I'm okay leaving as is.

1Copenut · 2022-05-05T21:52:15Z

src/components/call_out/call_out.styles.ts

+  return {
+    euiCallOutHeader: css`
+      font-weight: ${euiTheme.font.weight.medium};
+      margin-bottom: 0 !important; // In case it's nested inside EuiText


Sometimes an !important tag is what's needed.

1Copenut · 2022-05-05T22:06:39Z

src/components/call_out/call_out.tsx

      children,
      className,
-      heading,
+      heading = 'p',


What do you think about defaulting this heading to h2 ? If it won't cause a style break, I'd like to consider it for accessibility. Screen reader users often navigate pages asynchronously using headings and it'd be a nice addition. If p is the best default, I'm good leaving as is.

The problem is context. We have no idea where this will be placed (what if it came after an h3?). They're all over the place and my guess that could be a big breaking change in a11y tests.

But I agree p is not as good here and a heading. Can you make an issue to address this specifically. I don't want to make that call in a conversion PR.

Strong point about context. You're right, we don't know where it'll be used and we do not want to accidentally start nesting H2s inside H3, H4, etc. That'd be worse than no heading. I'll add a ticket to address this. Maybe as simple as an ahem, callout in our docs page that the consuming app should look at heading structure and use that as a guide for the prop value.

++ That and/or we can eventually make that property required by the consumer. That way they always have to make the consideration.

Filed #5884 for future work.

kibanamachine · 2022-05-06T15:20:19Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5870/

cchaos added 4 commits May 4, 2022 16:12

Fixed position logicals 😬

833f06d

[EuiCallOut] Initial conversion

c3bd617

Increased default text contrast value to 4.85

56bb294

Example to TS

783ad46

cchaos commented May 4, 2022

View reviewed changes

cl and deprecation notice

f209dd4

cchaos added the deprecations PRs that contains deprecations. Add them to the deprecations meta ticket after merge. label May 4, 2022

cchaos mentioned this pull request May 5, 2022

[Meta][CSS-in-JS] Component conversions #5685

Closed

49 tasks

elizabetdev self-requested a review May 5, 2022 15:18

elizabetdev approved these changes May 5, 2022

View reviewed changes

Update default prop logic for heading

8cde1f1

cchaos requested a review from 1Copenut May 5, 2022 18:53

1Copenut approved these changes May 5, 2022

View reviewed changes

ref -> panelRef

6a85444

cchaos enabled auto-merge (squash) May 6, 2022 15:15

1Copenut mentioned this pull request May 6, 2022

[EuiCallout][COGNITION]: Document consumer responsibility with header prop #5884

Closed

cchaos merged commit d6e6931 into elastic:main May 6, 2022

cchaos deleted the css-in-js/callout branch May 6, 2022 15:46

cchaos mentioned this pull request May 6, 2022

[EuiToast] Titles with icons don't wrap well #5111

Closed

breehall mentioned this pull request May 16, 2022

Upgrade EUI to v58.0.0 elastic/kibana#132257

Merged

thompsongl mentioned this pull request Jun 9, 2022

[EuiCallout] Set grow={false} #5963

Merged

9 tasks

	export const EuiCallOut = forwardRef<HTMLDivElement, EuiCallOutProps>(
	(
	{
	title,
	color = 'primary',
	size = 'm',
	iconType,
	children,
	className,
	heading,
	...rest
	},

Conversation

cchaos commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Also increased the default contrast ratio to 4.85

Checklist

Things to look out for when moving styles

QA

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented May 4, 2022

Uh oh!

elizabetdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented May 5, 2022

Uh oh!

cchaos commented May 5, 2022

Uh oh!

kibanamachine commented May 5, 2022

Uh oh!

1Copenut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented May 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cchaos commented May 4, 2022 •

edited

Loading

Also increased the default contrast ratio to `4.85`