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

Add EuiComment #3179

Merged
merged 27 commits into from
Apr 8, 2020
Merged

Add EuiComment #3179

merged 27 commits into from
Apr 8, 2020

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Mar 26, 2020

Summary

Add EuiComment component in preparation for the upcoming addition of EuiCommentList. Internally EuiComment uses two components EuiCommentTimeline and EuiCommentEvent.
There are two types of comments available: regular and update.

palpr

Desktop view
image

Mobile view

a_mobile

Notes:

  • We might need to further optimize this for Amsterdam Dark.
  • Pending: I’m still talking to @myasonik about a11y on this.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
    - [ ] Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Overall this is really great. Design wise I just spotted a few alignment and color issues, but largely looks awesome!

Component-wise I found a few places where it could be a bit smarter. And documentation-wise I think there should be more sections explaining the different pieces of the component. These are some example sections I'd add:

  • Basic example: What are all the bits that I can pass (username, event, timestamp, children)? Including Timeline icon: What's the default vs what consumers can pass in and recommended sizing.
  • Regular vs update types. What are the differences and why pick one over the other
  • Actions: What are they and why would I use them?

Design

I'd really like to see these line up a bit better on their baseline

Screen Shot on 2020-03-27 at 13:43:24

What do you think about lightening the background color of the default icon and matching the background color of the comment title?

Screen Shot 2020-03-27 at 12 55 08 PM

It looks like there's a rogue background color being added to the timeline icon.

Screen Shot 2020-03-27 at 12 48 40 PM

Take a look at the Amsterdam version and consider if you'd make any changes there.

Screen Shot 2020-03-27 at 13 55 43 PM

src-docs/src/views/comment/comment.tsx Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment.tsx Show resolved Hide resolved
src-docs/src/views/comment/comment.tsx Outdated Show resolved Hide resolved
src/components/comment/_comment.scss Outdated Show resolved Hide resolved
src/components/comment/_comment.scss Outdated Show resolved Hide resolved
src/components/comment/comment_timeline.tsx Outdated Show resolved Hide resolved
src/components/comment/comment_timeline.tsx Outdated Show resolved Hide resolved
src/components/comment/comment_list.tsx Outdated Show resolved Hide resolved
src/components/comment/comment_event.tsx Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment.tsx Outdated Show resolved Hide resolved
@thompsongl
Copy link
Contributor

Just an FYI, I'm going to hold off on reviewing until Caroline has finished her pass.

@kibanamachine
Copy link

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

@andreadelrio
Copy link
Contributor Author

What do you think about lightening the background color of the default icon and matching the background color of the comment title?

I gave this a go and it's in the current version (i.e. $euiColorLightestShade). While I think it looks great in light themes, I'm not 100% convinced it stands out enough in dark themes. See:

background-color: $euiColorLightestShade;
image

background-color: $euiColorLightShade;
image

@cchaos I've added some new sections, improved timelineIcon and optimized for Amsterdam. This should be ready for another round of review. Thanks!

@andreadelrio andreadelrio requested a review from cchaos April 2, 2020 05:18
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@andreadelrio andreadelrio requested a review from cchaos April 6, 2020 18:46
@myasonik
Copy link
Contributor

myasonik commented Apr 6, 2020

In the case of a regular comment or an update comment with a body, can we make them <figure> elements and the header string the <figcaption>?

Otherwise, in the case of an update comment without an body, can those be wrapped in a <p>?

cchaos and others added 2 commits April 6, 2020 16:39
@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

🎉 Looks great to me! All the browsers and mobile versions work now. All the tests are in place. Looks good in the themes. 🎉

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

This is fun!

Just one question/suggestion on timestamps, and some TS things. The API looks good

src-docs/src/views/comment/comment_example.js Outdated Show resolved Hide resolved
src/components/comment/comment_event.tsx Outdated Show resolved Hide resolved
src/components/comment/comment_timeline.tsx Outdated Show resolved Hide resolved
src/components/comment/index.ts Outdated Show resolved Hide resolved
src/components/comment/index.ts Outdated Show resolved Hide resolved
src/components/comment/comment_event.tsx Outdated Show resolved Hide resolved
src/components/comment/comment_timeline.tsx Outdated Show resolved Hide resolved
@andreadelrio
Copy link
Contributor Author

@thompsongl thanks for your review, good catch with the relative dates case. I made the changes you suggested. I also did some small changes to include Michail's a11y improvements as I think they make sense. However, the code that I added is repetitive. I tried making it better but ended up with TS errors. Can you please take a look here? And this is what I was trying:

  let Element = 'div';
  let HeaderElement = 'div';

  if (type == 'regular' || (type == 'update' && children)) {
    Element = 'figure';
    HeaderElement = 'figcaption';
  }

Which was giving me these errors:
image
image

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor

@myasonik

I'm working on the TS issues mentioned, but I'm not sure I fully understand what element should be the figure.

Just this?
Screen Shot 2020-04-08 at 10 29 37 AM

Or all of this?
Screen Shot 2020-04-08 at 10 29 09 AM

@andreadelrio
Copy link
Contributor Author

@myasonik

I'm working on the TS issues mentioned, but I'm not sure I fully understand what element should be the figure.

Just this?
Screen Shot 2020-04-08 at 10 29 37 AM

Or all of this?
Screen Shot 2020-04-08 at 10 29 09 AM

Just the side on the right (i.e. EuiCommentEvent)

@thompsongl
Copy link
Contributor

Updated figure vs div logic, and simplified the props of both EuiCommentEvent and EuiCommentTimeline. The latter bit is only possible because neither are exported as part of the public API. Also, it allows for easier interface extension in EuiComment.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

🎉

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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.

None yet

5 participants