Make icon area only clickable area in tags

[WalkingPizza]

What does it do?

Makes the icon area the only clickable area in Tag components.

Why is it needed?

At the moment clicking on the entire tags triggers the onClick action, when it should only happen when the icon is clicked.

How to test it?

Pass an onClick prop to a Tag component and notice how it does not trigger when anything but the icon area is clicked.

Related issue(s)/PR(s)

https://github.com/strapi/design-system/issues/679

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
design-system	✅ Ready (Inspect)	Visit Preview	Oct 14, 2022 at 10:49AM (UTC)
design-system-website	✅ Ready (Inspect)	Visit Preview	Oct 14, 2022 at 10:49AM (UTC)

[WalkingPizza]

PS: I also added a quick check to verify that the onClick prop is actually defined before calling it in handleClick.

[maevalienard]

Thanks for the update! Looks great to me on the design side, let's wait for a technical review now :)

[ilchenkoArtem]

PS: I also added a quick check to verify that the onClick prop is actually defined before calling it in handleClick.

Thanks. In current behavior (without this check) we have critical error in dev mode

[gu-stav]

@HichamELBSI I'll check with @maevalienard about what to do about the hover / focus state. So far it has been designed to work on the full element (which is not implemented):

With this change we could introduce it on the button only.

[gu-stav]

@WalkingPizza I've disussed the question about the :hover/ :focus state should look like with @maevalienard: we can dim down the icon a bit. The design-system Figma file is already updated: https://www.figma.com/file/PICiE8O4NLrHO1lhJftLdE/Strapi---UI-Kit-%F0%9F%A7%A9?node-id=10504%3A122030

Could you include these changes too?

[maevalienard]

@gu-stav The Figma link won't work because it's only open to Strapi employees :)

What is written in this file is that the icon should be in primary500 color when at hover state (primary600 is the default color).

[WalkingPizza]

I guess you'll have to hire me then 😂

Just to confirm, the changes also apply to the button when focusing it? Do I modify the rest of the tag so it stays in primary600 when being hovered/focussed?

[gu-stav]

@maevalienard Thanks, that's good to know. I thought the UI Kit is public: @WalkingPizza here the screenshot

So the icon is primary600 by default and primary500 for :hover/ :focus state

[WalkingPizza]

Have any changes been made to the disabled state? Do we still make the whole Tag gray-ish?

[gu-stav]

I don't find anything regarding this in the UI Kit and therefore I'd suspect you can keep the current styles. cc @maevalienard

[maevalienard]

Yes we can keep the current styles. Thanks! @WalkingPizza

@gu-stav Actually the UI Kit is public, but the access is different. One should duplicate the main file to have access to it :)

[WalkingPizza]

Added the actionLabel prop and the relative aria-label attributed and implemented the hover/focus states.

[HichamELBSI]

Added the actionLabel prop and the relative aria-label attributed and implemented the hover/focus states.

@WalkingPizza Thanks for the update, could you please apply the changes to the story as well?

[HichamELBSI]

@WalkingPizza Some tests are failing, please make sure to run the tests before (you may need to update the snapshots as well)

[WalkingPizza]

What exactly would you like to modify in the Tag's story?

[HichamELBSI]

What exactly would you like to modify in the Tag's story?

The action label please.

[WalkingPizza]

I suppose you mean here and here, but I have two questions:

1. Do we need an actionLabel even when onClick is not defined, since there is technically no action?
2. The Tag component is also used in this story, should we add an actionLabel there as well?

Regarding both (1) and (2), since no action is defined in onClick what do you suggest we use as label?

[gu-stav]

@WalkingPizza I just realized nobody got back to you regarding your questions. Sorry for that!

Do we need an actionLabel even when onClick is not defined, since there is technically no action?

I don't think so. What do you think about changing the tagname from button -> span (TagIconWrapper) of no onclick function was passed, so that the element isn't announced as such?

The Tag component is also used in this story, should we add an actionLabel there as well?

I don't think we have to, because without an onClick prop there won't be any action.

[WalkingPizza]

Since the button is being rendered as a span, is it semantically correct to still have the disabled and aria-disabled attributes in there?

I am asking because the color of the icon depends on the aria-disabled value, so removing it for semantical purposes would require a rework of the CSS too!

[gu-stav]

Good point. aria-disabled doesn't make sense in that case. 🤔 I guess we'd need to omit the attribute in that case. Regarding the styling

svg path {
    fill: ${({ theme, onClick, disabled }) => ((disabled || !onClick) ? theme.colors.neutral600 : theme.colors.primary600)};
  }

Does that make sense?