[EuiComment] 🛑 WIP Create styles header and border for `EuiCommentEvent` by breehall · Pull Request #7253 · elastic/eui

breehall · 2023-10-04T14:53:49Z

Summary

This PR extends the eventColor prop on EuiCommentEvent and applies the color to the entire comment header, not just the panel included inside of the header.

Before

After

QA

Toggle the eventColor prop within the Playground for EuiCommentEvent -> https://eui.elastic.co/pr_7253/storybook/index.html?path=/story/euicommentevent--playground

General checklist

Browser QA
- Checked in both light and dark modes
- Checked in mobile
- Checked in Chrome, Safari, Edge, and Firefox
- Checked for accessibility including keyboard-only and screenreader modes
Docs site QA
- Added documentation
- Props have proper autodocs (using @default if default values are missing) and playground toggles
- Checked Code Sandbox works for any docs examples
Code quality checklist
- Added or updated jest and cypress tests
Release checklist
- A changelog entry exists and is marked appropriately.
- If applicable, added the breaking change issue label (and filled out the breaking change checklist)
Designer checklist
- Updated the Figma library counterpart

…eader color to the entire comment header, not just the text background color

…e the color toggling

breehall · 2023-10-04T15:31:38Z

@andreadelrio @1Copenut

Hey Andrea & Trevor, do you all mind taking a look at the the change made to EuiCommentEvent in Storybook. After allowing extending the eventColor prop to set the header to our full range of colors (which is based on EuiPanel colors) I have accessibility concerns for the lighter shades. Do you all have any insight?

You can test this in Storybook by visiting the EuiCommentEvent example and toggling the eventColor prop. These colors match EuiPanel in production for light mode and dark mode, so this could extend a more broad conversation about those colors as well.

andreadelrio · 2023-10-04T18:15:45Z

@breehall I think we need darker colors for the borders.

breehall · 2023-10-04T18:32:16Z

Here are a few options I'm considering:

1Copenut · 2023-10-04T19:03:04Z

@breehall I think I lean toward option 1 for accessibility and visual interest. The higher contrast of the border makes the header more prominent and keeps us on the safe side of WCAG SC 1.4.11: Non-text contrast (AA). I did a thought experiment where I mocked up an alert with each of the brand color tints and full-strength border, then ran them through the axe-core browser plugin. We had zero contrast issues, and the strong borders really gave each their own character.

andreadelrio · 2023-10-04T19:34:26Z

Here are a few options I'm considering:

Thanks! Can you also give us something like 0.5, 0.75 to see how it feels?

breehall · 2023-10-04T19:38:13Z

@andreadelrio Here are a few more options in .5, .6, and .75

andreadelrio · 2023-10-04T19:56:47Z

@andreadelrio Here are a few more options in .5, .6, and .75

Thanks a lot! I'm partial to something between 0.3 and 0.5. I'd have to see all the other color variants to make a final recommendation.

breehall · 2023-10-04T20:42:29Z

@andreadelrio Here's a version of a comment with each color and each transparatize level to assist.

Transparatize @ .3

Transparatize @ .4

Transparatize @ .5

andreadelrio · 2023-10-04T20:52:27Z

@breehall this is super helpful thank you! My vote would be for 0.5. However, I feel like the warning border is too faint. Do we ever do a different treatment color for a specfic variant? In other words, could we make only the warning one darker? If the answer is "no" we could consider going up in the ratio for all the variants (>0.5). cc-ing @cee-chen in case they have input 🎶 (that last sentence was like a song in my head)

cee-chen · 2023-10-05T02:46:50Z

However, I feel like the warning border is too faint. Do we ever do a different treatment color for a specfic variant?

Yep, we totally do (e.g. success and accent colors for buttons). I think this is a warranted case.

One note - I'm slightly concerned by us using transparentize instead of lightenOrDarken (based on light vs dark mode) for background colors. It isn't possible to correctly calculate contrast ratios for colors with alpha channels. Can we consider tinting or shading instead of transparentizing?

- Create DRY style helper for border styles - Update styling conditions in CommentEvent to add a matching border when eventColor is passed in and the comment is of the *regular* type

…r event borders for testing

breehall · 2023-10-05T16:10:38Z

@andreadelrio Do you mind going to the docs staging for EuiComment and let me know what you think about these colors? Feel free to toggle between light and dark as well. I realized the changes look different in images because I've zoomed out in my browser, so the colors in the images don't 100% reflect the code change.

Staging: https://eui.elastic.co/pr_7253/#/display/comment-list#basic-comment-list

andreadelrio · 2023-10-05T18:34:15Z

@andreadelrio Do you mind going to the docs staging for EuiComment and let me know what you think about these colors? Feel free to toggle between light and dark as well. I realized the changes look different in images because I've zoomed out in my browser, so the colors in the images don't 100% reflect the code change.

Staging: https://eui.elastic.co/pr_7253/#/display/comment-list#basic-comment-list

@breehall I just checked it out, I like it! Question, what are the differences between subdued, plain and transparent? do we need all of them?

breehall · 2023-10-05T18:52:08Z

what are the differences between subdued, plain and transparent? do we need all of them?

@andreadelrio Subdued is barely a shade lighter than the plain/transparent variations (which are the same). I didn't add these colors to the prop, but if I had to guess, I assume these were all added because the prop is passed directly to the EuiPanel inside of EuiCommentEvent.

A quick check in Kibana showed subdued is being used twice, but I don't see usages for plain or transparent. Also nice to note, this prop doesn't have a default.

andreadelrio · 2023-10-05T20:50:02Z

src/components/comment_list/comment_event.styles.ts

+    euiCommentEvent__header: css``,
+    // types
+    regular: css`
+      background: ${euiTheme.colors.lightestShade};


@breehall could we get rid of this and add a default eventColor to this component instead?

I actually think this might be better than adding the color as a component default. The reason I say this is because a background is only set on type regular.

The other two types (custom and update) don't have a background color. So we would be setting the default, and then unsetting if the type is custom or update instead of the above approach which is setting the background color only for type regular or applying the eventColor if needed.

andreadelrio · 2023-10-05T20:52:35Z

EuiComment has changed a ton since I last contributed to it so I'm not clear on all the inner workings of it. Having said that, my take is that we only need one gray color (which could be subdued) and we also need to support transparent for when the type is update.

breehall · 2023-10-06T14:31:53Z

@andreadelrio

my take is that we only need one gray color (which could be subdued) and we also need to support transparent for when the type is update.

Below is the logic we currently have. If an event color is passed view EuiComment props, we pass that directly to the EuiPanel. If plain is passed, we actually default to transparent because plain gives us a shadow that we don't want on the panel.

This means we have subdued as a gray color and transparent appears as a gray color because the comment header background uses euiTheme.colors.lightShade. If we would like to only have one gray, we could change the regular CSS to use the subdued styling instead of euiTheme.colors.lightShade. What do you think of that? @cee-chen do you have any thoughts?

  // The 'plain' color creates a shadow and adds a border radius that we don't want.
  // So for these cases we use the transparent color instead.
  const finalEventColor = eventColor === 'plain' ? 'transparent' : eventColor;

…olors for event borders for testing" This reverts commit e5341a2.

breehall · 2023-10-06T19:17:52Z

src/components/comment_list/comment_event.styles.ts

+      return `1px solid ${tintOrShade(
+        euiTheme.colors.accent,
+        ratio,
+        colorMode


I tried to DRY this out by simply returning (1px solid ${tintOrShade(euiTheme.colors[color], ratio, colorMode) to avoid logic inside of the switch, but that made TypeScript angry.

breehall · 2023-10-06T19:18:37Z

src/components/comment_list/comment_event.styles.ts

-  custom: css``,
-});
+const _generateEventBorderColor = (
+  { euiTheme, colorMode }: UseEuiTheme,


This function accepts colorMode on the chance that we need different color ratios for light and dark mode, but if the 0.6 ratio is OK for both, we can get rid of it.

breehall · 2023-10-06T19:20:39Z

src/components/comment_list/comment_event.styles.ts

+    hasEventColor: css`
+      padding: 0;
+    `,
+    border: {


border doesn't need to be its own object if you'd prefer to leave the colors as a direct child of the main styling object. I felt like it was easier to contain these styles in border{} so we're aware that these only apply to borders.

kibanamachine · 2023-10-06T19:45:31Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7253/
Storybook: https://eui.elastic.co/pr_7253/storybook

elasticmachine · 2023-10-06T19:49:04Z

💚 Build Succeeded

Buildkite Build
Commit: 762ded3

History

💔 Build #773 failed e5341a2
💚 Build #757 succeeded f6c139e

cee-chen · 2023-10-17T18:46:17Z

Closing in favor of #7288

breehall added 2 commits October 4, 2023 10:50

[EuiCommentEvent] Extended the eventColor prop to apply the desired h…

e0a2a4e

…eader color to the entire comment header, not just the text background color

Storybook - Create simple storybook for EuiCommentEvent to demonstrat…

f6c139e

…e the color toggling

breehall changed the title ~~[EuiComment] Create styles header and border for EuiCommentHeader~~ [EuiComment] Create styles header and border for EuiCommentEvent Oct 4, 2023

breehall changed the title ~~[EuiComment] Create styles header and border for EuiCommentEvent~~ [EuiComment] WIP Create styles header and border for EuiCommentEvent Oct 4, 2023

breehall added 3 commits October 5, 2023 11:32

[EuiComment]

80bdfa0

- Create DRY style helper for border styles - Update styling conditions in CommentEvent to add a matching border when eventColor is passed in and the comment is of the *regular* type

Update test cases

abb1f55

[DO NOT MERGE] Added a doc with the different variations of colors fo…

e5341a2

…r event borders for testing

breehall changed the title ~~[EuiComment] WIP Create styles header and border for EuiCommentEvent~~ [EuiComment] 🛑 WIP Create styles header and border for EuiCommentEvent Oct 5, 2023

andreadelrio reviewed Oct 5, 2023

View reviewed changes

Revert "[DO NOT MERGE] Added a doc with the different variations of c…

762ded3

…olors for event borders for testing" This reverts commit e5341a2.

breehall commented Oct 6, 2023

View reviewed changes

andreadelrio mentioned this pull request Oct 16, 2023

[EuiComment] Create styles header and border for EuiCommentEvent #7288

Merged

10 tasks

cee-chen closed this Oct 17, 2023

Conversation

breehall commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Uh oh!

breehall commented Oct 4, 2023

Uh oh!

andreadelrio commented Oct 4, 2023

Uh oh!

breehall commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1Copenut commented Oct 4, 2023

Uh oh!

andreadelrio commented Oct 4, 2023

Uh oh!

breehall commented Oct 4, 2023

Uh oh!

andreadelrio commented Oct 4, 2023

Uh oh!

breehall commented Oct 4, 2023

Uh oh!

andreadelrio commented Oct 4, 2023

Uh oh!

cee-chen commented Oct 5, 2023

Uh oh!

breehall commented Oct 5, 2023

Uh oh!

andreadelrio commented Oct 5, 2023

Uh oh!

breehall commented Oct 5, 2023

Uh oh!

andreadelrio Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

breehall Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

andreadelrio commented Oct 5, 2023

Uh oh!

breehall commented Oct 6, 2023

Uh oh!

breehall Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

breehall Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

breehall Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 6, 2023

Uh oh!

elasticmachine commented Oct 6, 2023

💚 Build Succeeded

History

Uh oh!

cee-chen commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

breehall commented Oct 4, 2023 •

edited

Loading

breehall commented Oct 4, 2023 •

edited

Loading