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

Make clear ActionList.Item's trailingIcon can be a component #1435

Closed
six7 opened this issue Sep 17, 2021 · 8 comments
Closed

Make clear ActionList.Item's trailingIcon can be a component #1435

six7 opened this issue Sep 17, 2021 · 8 comments
Labels

Comments

@six7
Copy link
Contributor

six7 commented Sep 17, 2021

The component props of ActionList.Item suggest that trailingIcon can be either an icon or similar.

  /**
   * Icon (or similar) positioned after `Item` text.
   */
  trailingIcon?: React.FunctionComponent<IconProps>

Would a Label be considered appropriate usage for this? If so, should we update props to allow more generic usage?

In addition to this, leadingVisual has the same definition but it's name ("Visual") suggest a more generic usage, I'm wondering if this differentiation in name is still appropriate.

  /**
   * Icon (or similar) positioned before `Item` text.
   */
  leadingVisual?: React.FunctionComponent<IconProps>

In effect, this is what I'm envisioning:

image

@colebemis
Copy link
Contributor

Thank you! I added this to our project board.

@broccolini
Copy link
Member

This may just be a renaming issue, it sounds like the team were able to implement with existing functinoality. @jfuchs will confirm, and @vdepizzol will review api naming.

@dusave
Copy link
Contributor

dusave commented Sep 20, 2021

@jfuchs and I worked and found that it actually is supported, but the naming and the prop restrictions (IconProps) were confusing. This is no longer a blocker, but it would be nice to make it clear (if Primer wishes to support it) that it supports React Components as either Leading or Trailing elements

@vdepizzol vdepizzol changed the title Allow ActionList.Item's trailingIcon to be a component Make clear ActionList.Item's trailingIcon can be a component Sep 20, 2021
@vdepizzol
Copy link
Contributor

@six7 @dusave @colebemis I took the permission to rename this issue to address the API naming specifically, since custom components are already supported. ✨

Some background on current props and behaviors:

  1. Currently we have leadingVisual to represent the left-aligned section. So far we only support 16px visuals, being icons or avatars or other custom visuals. Originally this was also scoped to support 20px, with planned support for more custom sizes in the future. In terms of naming, I think "visual" works here since it's always referring to a compact, icon-like square area.

  2. For the right-aligned section, as documented in the design specs, we can have icons, counters, custom-styled text, and keyboard shortcuts being applied here. In the API right now we have both trailingIcon and trailingText, the first supporting any component, and the latter only accepting strings.

  3. In theory both trailingIcon and trailingText can be used together (I could picture a case of an actionList item with a right arrow on the right-most corner, with a trailing text on its side describing a selected value). Testing that quickly shows that there's no spacing between the two elements, though.

Options

We could adopt trailingVisual for the right-aligned component, making it obvious that side area can be used for more components than just icons. That's also how we refer to it in the design docs, even though they are not exactly the same as leadingVisual in terms of behavior (one being always in a square container, the other supporting flexible widths).

If we think there's a need to make it more accurate, we could name it to something more generic, like trailingArea or simply trailing, but I think that could be even more confusing as having visual to the prop may help identifying what it ultimately does ✌️.

@jfuchs @colebemis @six7 thoughts?

A draft of tasks discussed above

  • Rename trailingIcon to trailingVisual
  • Fix horizontal spacing when using trailingVisual and trailingText are used in tandem (8px as usual)
  • How to support multiple different leadingVisual sizes? (at least add support for 20px avatars to start besides our current 16px icons)

@dusave
Copy link
Contributor

dusave commented Sep 20, 2021

Just my two cents as a consumer, trailingComponent would make it clear as well as use predefined nomenclature. WRT the leading, is there a reason to restrict the widths? Ideally, it would be square, but that could be handled by the consumer (if it's an icon, that sizing is handled by the icon component itself).

Honestly, an even more approachable.... approach, would be to allow the contents of the cell to be defined, but without requiring all iterations to be redefined (e.g. hover interactions, colors, dividers, etc.) When we didn't know we could use the trailingIcon prop, I started to go down this route and quickly got overwhelmed with the amount of logic and styling that I would need to handle myself.

@six7
Copy link
Contributor Author

six7 commented Sep 21, 2021

We could adopt trailingVisual for the right-aligned component, making it obvious that side area can be used for more components than just icons. That's also how we refer to it in the design docs, even though they are not exactly the same as leadingVisual in terms of behavior (one being always in a square container, the other supporting flexible widths).

👍 to trailingVisual for consistency with existing docs and other props. Also, we're currently treating that prop with IconProps - I think we should update that as well to reflect a more generic usage?

@vdepizzol
Copy link
Contributor

vdepizzol commented Oct 4, 2021

FYI we're adopting trailingVisualto be used in the ActionList component in Primer CSS — @langermank and I have been working on it here, with a PR to primer/css here.

@gaknoll gaknoll added the react label Oct 19, 2021
@siddharthkp
Copy link
Member

siddharthkp commented Oct 26, 2021

Implemented in #1521 / v31.0.1 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants