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

chore: adapt Tag to design #693

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

RamProg
Copy link
Contributor

@RamProg RamProg commented Oct 14, 2024

🚪 Why?

Update tag profile

🔑 What?

Add feedback for hover

🏡 Context

https://www.figma.com/design/uImdC0GKxH59DkjzBqRJEF/Layouts-%26-Blocks?node-id=1469-190164&node-type=frame&t=dm9bATe2HlYwytsm-0

Hover state:
image

@RamProg RamProg self-assigned this Oct 14, 2024
@RamProg RamProg requested a review from a team as a code owner October 14, 2024 15:35
Copy link

swarmia bot commented Oct 14, 2024

✅  Linked to FCT-19788 · Create Link Label
➡️  Part of Epic FCT-16894 · Web - Employee Profile MVP (Overview)

Copy link

github-actions bot commented Oct 14, 2024

🔍 Visual review for your branch is published 🔍

Here are the links to:

@eliseo-juan
Copy link

It's hard to me to keep calling this component Tag, maybe it's a selectable/navigable row? I know this is gonna be on experimental, but the name "tag" doesn't work well for this component

@RamProg
Copy link
Contributor Author

RamProg commented Oct 14, 2024

It's hard to me to keep calling this component Tag, maybe it's a selectable/navigable row? I know this is gonna be on experimental, but the name "tag" doesn't work well for this component

Last time we reviewed these elements they were called Tags in the design system in Figma. I could create a wrapper to make it clickable, but I think this just adds feedback (bg-color and icon) on a feature that was already there and I don't see anywhere else where we could use the wrapper.

@@ -23,21 +25,27 @@ export const Tag = forwardRef<HTMLDivElement, TagProps>(
return (
<div
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we make it a button or an a? this will help us dropping the !!onClick check in favor of disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that if doesn't have an onClick action then it's not really a button or an a so then it would be semantically wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we should probably change the HTML element conditionally for accessibility purposes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I see you're changing the role instead. It's probably ok then!

@RamProg RamProg enabled auto-merge (squash) October 15, 2024 09:51
@RamProg RamProg disabled auto-merge October 16, 2024 07:14
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.

4 participants