carbon-design-system / carbon

A design system built by IBM
https://www.carbondesignsystem.com
Apache License 2.0
7.86k stars 1.82k forks source link

Tag: when a Tag is actionable(has an onClick), the pointer icon does not change #7592

Closed guigueb closed 3 years ago

guigueb commented 3 years ago

Title line template: Tag: when a Tag is actionable(has an onClick), change the pointer icon

What package(s) are you using?

Detailed description

When a Tag has a filter property, the mouse icon will change when hovering over the Tag. <Tag type="red" title="Clear Filter" filter> Red </Tag> If a Tag does not have the filter property but does have an actionable property(onClick), the mouse will NOT change when hovering over the Tag. <Tag type="red" title="Clear Filter" onClick={()=>alert("red")}> Red </Tag>

I expected the mouse icon to change when actionable in the same way as when filter property is set.

This happens in any browser. This happens in any version of the Carbon Design System.

Steps to reproduce the issue

  1. Create a tag like <Tag type="red" title="Clear Filter" filter> Red </Tag>
  2. Create a tag like <Tag type="red" title="Clear Filter" onClick={()=>alert("red")}> Red </Tag>
  3. Hover over each and observe the icon changing(or not).

Use case

The product has a search input. The product has three suggestions for searching "Platform", "Runtimes", and "Services". It shows these as clickable tags for quick filtering. This is on the Carbon Design System Tag page just above the Live Demo. https://www.carbondesignsystem.com/components/tag/usage#live-demo

Work arounds

The child of Tag is a node, so it could be coded as an input. Styling could be applied, to account for the hover. Always create a filter tag - and apply display: none; to the filter button when not a filter tag. All of these are client work and they come with their own problems.

dakahn commented 3 years ago

confirmed the above described behavior.

dakahn commented 3 years ago

@carbon-design-system/design is the intention that tags are used like buttons as described above?

guigueb commented 3 years ago

Yes, exactly - we display tags which have an onClick action similar to buttons. Our usage looks similar to the tags in the "Data table" and "search input" on the https://www.carbondesignsystem.com/components/tag/usage page. Where clicking on a tag will perform an action like showing properties related to a tag, or filter by a tag, etc. This behavior currently works great - but the icon doesn't indicate the tag is actionable.

emyarod commented 3 years ago

cc @carbon-design-system/design does the default Tag support click actions or is that specific to Filter Tags only?

dakahn commented 3 years ago

this was briefly discussed on Slack, seems like a proposal/feature enhancement to me. Converting and moving to the right pipeline

guigueb commented 3 years ago

Any Tag (default or with a filter prop) supports and actionable callback (like onClick). A Tag with a filter prop....

I would suggest that the mouse icon change should be tied to having an actionable callback (like onClick) not to the flag filter prop.

dakahn commented 3 years ago

@guigueb yeah, although onClick works for non-filter tags, it seems like from a design spec standpoint, that's not a intended use case at the moment.

Hover, active, focus states need to be determined etc etc. 👍🏽

guigueb commented 3 years ago

@dakahn

Shouldn't all the hover, active, focus states already be in place for a filter tag - as it is an actionable tag? I was hoping/expecting to simply apply any/all the same styling for actionable tags as you would for filter tags.

dakahn commented 3 years ago

the states are in place for the filter variant which has specific use cases and guidance. As it stands a clickable action on a non-filter tag isn't something we support from a design system standpoint even though as you point out the API seems to support this behavior.

guigueb commented 3 years ago

OK. A work around is to create a filter tag - and apply display: none; to the filter button.

tw15egan commented 3 years ago

This also goes hand in hand with https://github.com/carbon-design-system/carbon/issues/7160, as click events are passed directly to the Tag through the ...other props.

PaulJThompson commented 3 years ago

In the prior version of Cognos Analytics, the cursor does change when a tag has an action associated with it. From our point of view, it is a regression that Carbon does not support this.

aagonzales commented 3 years ago

The default tag should continue to not have a cursor change. However, we can add a new variant for interactive tag that has a hover background color change and a cursor change.

I'm curious what the use case for this is and what other states might be needed. Does there also need to be a selected state? What kind of actions are you attaching to the tag?

guigueb commented 3 years ago

agreed - The default tag should continue to not have a cursor change.

I'm not sure a new variant is needed - just have the tag be aware of any onXXX events it may have. (I'm pretty sure the filter prop is not needed - you could have just checked for an onClose callback) If the tag contains a onClick, onClose, etc... then its actionable and the Carbon states (hover/focus/etc) should apply.

We don't use a selected state... but I suppose that could be an enhancement. Selected state would require additional props.

I am only aware of using the actions onClick and onClose, but who's to say what users will add. But now that you brought it up, we should also have a tabIndex and onKeyDown added to the Tabs for keyboard navigation... I updated one of the Tags in the sandbox example with this.

This sandbox example shows how we use it... https://codesandbox.io/s/actionable-tag-icon-does-not-change-3fpff

The Carbon tag page https://www.carbondesignsystem.com/components/tag/usage has a non-working example of pretty much what we are doing. image

aagonzales commented 3 years ago

Ok what I'm seeing the sandbox seems ok with me then. I was thinking more of a "selectable" which would be another variant.

guigueb commented 3 years ago

@tw15egan @emyarod @aagonzales

I know this is closed and it does address the onClick event. But it doesn't address any other actionable event - ondblclick, ondrag, onfocus, ...

I suppose it can be faked by adding a no-op onClick event = { onClick={() => {}} } to get the visible changes while doing the other action but it seems kinda silly.

Is there a reason why we choose to only deal with the onClick event? If not, should I log another enhancement?

emyarod commented 3 years ago

can you elaborate on what you mean by addressing other actionable events? they should already be spread into the component and the interactive outline appears on focus and clicks

guigueb commented 3 years ago

The PR handled onClick but not other actionable events like onDoubleClick, onMouseOver, etc I'm not 100% on the onMouseOver but the onDoubleClick should have the UI feedback like onClick. Basically, this issue was to handle all actionable events (not 100% sure what they all are - but it only handles onClick).

Either they should be allowed - and all give UI feedback, or they should be disallowed and not do anything.

// addressed <Tag type="red" title="Clear Filter" onClick={() => console.log("onClick")}> Red // not addressed - and there are likely others <Tag type="magenta" title="Clear Filter" onDoubleClick={() => console.log("onDoubleClick")}> Magenta <Tag type="purple" title="Clear Filter" onMouseOver={() => console.log("onMouseOver")}> Purple

emyarod commented 3 years ago

the PR modified the cursor when the component is interactive, and the component already had focus and hover styles which overlap with onMouseOver, onDoubleClick, etc.

are you suggesting that there should be additional visual feedback on double clicks and mouseovers?

although I am assuming from your code snippet that you are implying that these events are not handled? that should not be the case in the current component, but I will need additional clarification from you about that

image

guigueb commented 3 years ago

Q) are you suggesting that there should be additional visual feedback on double clicks and mouseovers? A) yes, I'm not 100% on the onMouseOver but the onDoubleClick should have the UI feedback like onClick.

And I'm pretty sure there are other actionable events that need to be considered.

From the PR only onClick flags the interactive tag. image

This sandbox will show that only onClick is handled.. https://codesandbox.io/s/actionable-tags-qxqyj While all three have actionable event handlers only the onClick Tag has UI indicating to indicate that.

emyarod commented 3 years ago

I'm not sure if double click interactions are part of the design system (cc @aagonzales) but the workaround here of course is to manually add the interactive class. on a general note, we typically do not modify styles based on JS event handlers outside of ones that coincide with the hover, focus, etc. CSS pseudoclasses, and I think it would be unreasonable to include the entire list of JS events here to get the cursor: pointer style rule

on a side note, I am not certain if double click interactions are fully supported by screen readers

guigueb commented 3 years ago

Agreed, which is why I initially suggested an 'isInteractive' property. As it is, a onClick no-op will work as an 'isInteractive' property, kludgy but it will work :-)

<Tag
      type="magenta"
      title="onDoubleClick hacked with an onClick no-op"
      onDoubleClick={() =>
        console.log("onDoubleClick hacked with an onClick no-op")
      }
      onClick={() => {}} // <<<=====  added just to get the interactive UI settings
>
      onDoubleClick hacked with an onClick no-op
</Tag>
DragosRistici commented 2 years ago

Double-clicking typically selects a paragraph or line of text and HTML content. You can "double-click" icons of apps to open them, even on web, which is a behavior imported from the desktop apps. Also you could "double-click" to perform cell-based interactions of web tables, but here usually you have 2 actions back-to-back: selecting the cell with the first click and entering edit mode with the second, or selecting the app icon with the first and opening it with the second (On the accessibility part, its Tab for selection and Enter/Return for the action).

I didn't find any submitting actions through buttons that use a hover and also a double-click event on the web. I'm sure there are, but I don't believe that's the norm. Clicking usually means a change in the webpage, either through hyperlinks or submit buttons. Hovering is used to show the responsiveness to the clicking of different elements on the screen: hyperlinks, drop-down menus, buttons, etc.

I think it would be tricky from a user experience point of view to try to override the double click behavior of the browser.

So for clickable tags, if they have the hover/focus/active/etc usual button states, the double-click should be treated just like the regular button treats it. Double-clicking a tag feels like should be a visual feature for a CMS admin in the add/edit/delete tag nomenclator functionality that opens the tagName edit feature.