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

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

Merged
merged 22 commits into from
Oct 18, 2023

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Oct 16, 2023

Summary

Continuation of the work started by Bree Hall in #7253
Closes #7238

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. This also updates the color transparent to EuiComment of type regular.

EuiComment type regular, eventColor="danger"

Before
image

After
image

EuiComment type regular, eventColor="transparent"

Before
image
After
image

I also updated the Comment section in the docs to include the eventColor prop and indicate the default color for comments of type regular and update.

image image image

QA

Remove or strikethrough items that do not apply to your PR.

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
  • Code quality checklist
  • 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

breehall and others added 9 commits October 4, 2023 10:50
…eader color to the entire comment header, not just the text background color
- 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
…olors for event borders for testing"

This reverts commit e5341a2.
@andreadelrio andreadelrio changed the title Comment list colors [EuiComment] Create styles header and border for EuiCommentEvent Oct 16, 2023
@andreadelrio andreadelrio marked this pull request as ready for review October 16, 2023 04:34
@andreadelrio andreadelrio requested a review from a team as a code owner October 16, 2023 04:34
- by setting border not via the shorthand property
+ simplify padding logic generally to exist on the `EuiPanel`, not on the header wrapper

+ code organization w/ comments
+ clean up storybook - add a story for each type, and make the example action change in color as well
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS/JS cleanups have been pushed up - I just have two questions remaining for you @andreadelrio!

src/components/comment_list/comment_event.tsx Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment_list.tsx Outdated Show resolved Hide resolved
- the `actions` control for JSX is very annoying, this makes it take up less space on the pane and also super easy to toggle on/off
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen
Copy link
Member

cee-chen commented Oct 18, 2023

Was waiting for staging to do one final QA pass! I think may just found a regression(?) with the update comments in how the avatar is aligned:

screencap.mov

Compared to prod: https://eui.elastic.co/#/display/comment-list#comment

The avatar now jumps a bit due to the extra padding added for the new bg colors because the previous colored example didn't have any icons, and the new one does. @andreadelrio any preference on this?

  1. Vertically align the avatar to the top instead of to the center
  2. Reduce the panel padding from 8px to 4px so the avatar doesn't jump
  3. Leave it as it is, as this likely won't be noticeable to actual production use cases

@andreadelrio
Copy link
Contributor Author

andreadelrio commented Oct 18, 2023

The avatar now jumps a bit due to the extra padding added for the new bg colors because the previous colored example didn't have any icons, and the new one does. @andreadelrio any preference on this?

  1. Vertically align the avatar to the top instead of to the center
  2. Reduce the panel padding from 8px to 4px so the avatar doesn't jump
  3. Leave it as it is, as this likely won't be noticeable to actual production use cases

Nice catch! I would go with #3 with #1 as my second option.

@cee-chen
Copy link
Member

Leaving it as-is makes sense to me, just wanted to make sure it was a deliberate choice! Everything else looks fantastic - let's ship it!

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@andreadelrio andreadelrio merged commit 6f5d004 into elastic:main Oct 18, 2023
7 checks passed
cee-chen added a commit to elastic/kibana that referenced this pull request Nov 3, 2023
`v89.1.0`⏩`v90.0.0`

The majority of changes in this PR come from:

- **EuiContextMenu** being converted to Emotion
(elastic/eui#7312). If your usage of
`EuiContextMenu` was significantly affected, we recommend pulling down
this PR and QAing it locally.

- `defaultProps` being removed from some very widespread components,
particularly **EuiButton**, in anticipation of React's upcoming
deprecation.
(elastic/eui@b7dc9b4)
**NOTE**: This only affected Enzyme snapshots, and did not affect
production behavior.

[Commits](https://github.com/elastic/kibana/pull/170179/commits) have
been broken up by component changes as well as types of changes.

---

## [`90.0.0`](https://github.com/elastic/eui/tree/v90.0.0)

- Updated the `eventColor` prop on `EuiCommentEvent` to apply the color
to the entire comment header.
([#7288](elastic/eui#7288))
- Updated `EuiBasicTable` and `EuiInMemoryTable` to support a new
controlled selection API: `selection.selected`
([#7321](elastic/eui#7321))

**Bug fixes**

- Fixed controlled `EuiFieldNumbers` not correctly updating native
validity state ([#7291](elastic/eui#7291))
- Fixed `EuiListGroupItem` to pass `style` props to the wrapping `<li>`
element alongside `className` and `css`. All other props will be passed
to the underlying content.
([#7298](elastic/eui#7298))
- Fixed `EuiListGroupItem`'s non-transitioned transform on hover/focus
([#7298](elastic/eui#7298))
- Fixed `EuiDataGrid`s with `gridStyle.stripes` sometimes showing buggy
row striping after being sorted
([#7301](elastic/eui#7301))
- Fixed `EuiDataGrid`'s `gridStyle.rowClasses` API to not conflict with
`gridStyle.stripes` if dynamically updated
([#7301](elastic/eui#7301))
- Fixed `EuiDataGrid`'s `gridStyle.rowClasses` API to support multiple
space-separated classes
([#7301](elastic/eui#7301))
- Fixed `EuiInputPopover` not calling `onPanelResize` callback prop
([#7305](elastic/eui#7305))
- Fixed `EuiDualRange` incorrectly positioning highlights when rendered
with `showInput="inputWithPopover"`
([#7305](elastic/eui#7305))
- Fixed `EuiTabs` incorrectly wrapping text when it should instead
either scroll or truncate
([#7309](elastic/eui#7309))
- `EuiContextMenu` now renders text colors correctly when used within an
`EuiBottomBar` ([#7312](elastic/eui#7312))
- Fixed the width of `EuiSuperDatePicker`'s Absolute date picker
([#7313](elastic/eui#7313))
- Fixed `EuiDataGrid` cells visually cutting off overflowing content a
little too quickly ([#7320](elastic/eui#7320))

**Deprecations**

- Deprecated `EuiBasicTable` and `EuiInMemoryTable`'s ref `setSelection`
API. Use the new `selection.selected` API instead.
([#7321](elastic/eui#7321))

**Breaking changes**

- Removed `EuiPageTemplate_Deprecated`, `EuiPageSideBar_Deprecated`, and
`EuiPageContent*_Deprecated`
([#7265](elastic/eui#7265))
- Removed the `ghost` color option from `EuiButton`, `EuiButtonEmpty`,
and `EuiButtonIcon`. Use an `<EuiThemeProvider colorMode="dark">`
wrapper and `color="text"` instead.
([#7296](elastic/eui#7296))

**Dependency updates**

- Updated `refractor` to v3.6.0
([#7127](elastic/eui#7127))
- Updated `rehype-raw` to v5.1.0
([#7127](elastic/eui#7127))
- Updated `vfile` to v4.2.1
([#7127](elastic/eui#7127))

**Accessibility**

- `EuiContextMenu` now correctly respects reduced motion preferences
([#7312](elastic/eui#7312))
- `EuiAccordion`s no longer attempt to focus child content when the
accordion is externally opened via `forceState`, but will continue to
focus expanded content when users click the toggle button.
([#7314](elastic/eui#7314))

**CSS-in-JS conversions**

- Converted `EuiContextMenu`, `EuiContextMenuPanel`, and
`EuiContextMenuItem` to Emotion; Removed `$euiContextMenuWidth`
([#7312](elastic/eui#7312))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiComment] Allow comment header to take a styled message color and border
5 participants