design icon indicating copy to clipboard operation
design copied to clipboard

Published 20 hours ago • verified publisher icon primer

Comment box

Open simurai opened this issue 3 years ago • 5 comments

This adds the CommentBox component.

👀 Preview

A diagram of a comment box with a label, a tab nav, a toolbar and an input with a placeholder text.
  • Figma source (for the images)
  • Part of https://github.com/github/primer/issues/737
  • API

simurai avatar Sep 22 '22 07:09 simurai

Very neat. 👍

Are we removing the Attach files by dragging ... hint below the input?

lukasoppermann avatar Sep 22 '22 08:09 lukasoppermann

Are we removing the Attach files by dragging ... hint below the input?

Yes, that's the plan. It's a bit risky because it helps new users discover that feature, but once you learned about it, it's also a bit waste of space to always show it. I was thinking that we might could surface it as part of the "pro tips" that are shown in issues/PRs, but it would not be a permanent part of the component.

Here a CodePen with an example that links to the docs, but that could also be more detailed. Or we keep rotating pro tips randomly.

Update: Added an "Example usage" section that includes a pro tip. Exact wording is still to be decided:

pro tip

simurai avatar Sep 22 '22 12:09 simurai

@simurai looking great! quick quick comments:

  • I think icon buttons have a more subtler color other than fg.default?

  • We probably need to confirm the padding around the icons to make sure there's enough touch area. Particularly in the narrow representation. This probably means 8px of padding around 32px-tall controls. Reference:

    fine vs coarse touch target on icon buttons

  • Instead of having a section called "Responsiveness", I think we can use "Responsive behavior"?

vdepizzol avatar Sep 23 '22 17:09 vdepizzol

@vdepizzol * I think icon buttons have a more subtler color other than fg.default?

Was wondering about that too for ActionBar in this comment.

Right. In .btn-octicon we use fg-muted. But for the new .Button--invisible (that is used in ActionBar) it's fg-default.

@langermank do you happen to remember if using fg-default for .Button--invisible was intentional? Possibly accessibility related? Since .Button--invisible shows an icon stand-alone (no border and no other text), it should use the default text color?

Update: Ok, changed it to fg.muted in https://github.com/primer/design/pull/298/commits/ad6b3a9a49f42bbbf90ce2fceab8a1f9dbebfb20

simurai avatar Sep 26 '22 05:09 simurai

@vdepizzol * We probably need to confirm the padding around the icons to make sure there's enough touch area. Particularly in the narrow representation. This probably means 8px of padding around 32px-tall controls.

Is an 8px gap also needed for controls that have no border? This came up in the ActionBar API too https://github.com/github/primer/pull/1236#discussion_r966668427:

We could, but I think having a gap between Button--invisible is not really needed since they have no borders and visually there is already enough spacing between the icons. With an extra gap the icons would feel too spaced out:

gap: 0:

Screen Shot 2022-09-09 at 15 01 04

gap: 8px:

Screen Shot 2022-09-09 at 15 01 33

But yes, for pointer: coarse there is an extra gap to account for the 44px min target area.

Instead of having a section called "Responsiveness", I think we can use "Responsive behavior"?

👍

simurai avatar Sep 26 '22 05:09 simurai

Gonna merge this as is since we're already testing the CommentBox in a prototype and the Primer Web implementation is published. There will likely be follow-up changes in the future.

simurai avatar Oct 21 '22 02:10 simurai