Skip to content

Table: omit opacity on IconButton in TableRow #566

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

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

Conversation

natalyjazzviolin
Copy link
Contributor

@natalyjazzviolin natalyjazzviolin commented Mar 12, 2025

Closes #565

Why

If a row is set to isDisabled or isDeleted, the opacity on the row is set to 0.5. This causes all children of the TableRow to be greyed out and makes it unclear that the TableRowCloseButton icon is actionable:
Screenshot 2025-03-12 at 11 29 21 AM

What

  • Sets the TableRowCloseButton color to the default theme color
  • Removes opacity styling from TableRow
  • Adds conditional opacity to SelectData (checkbox) and TableData, while leaving opacity: 1 on the TableRowCloseButton
Screenshot 2025-03-12 at 4 22 20 PM

I left the TableRowCloseButton icon to use type="ghost because the primary button type has a different styling onclick:
https://github.com/user-attachments/assets/bdd0e16c-a92c-4286-9890-26ca087347ac
Screenshot 2025-03-12 at 4 24 54 PM

Copy link

vercel bot commented Mar 12, 2025

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

Name Status Preview Comments Updated (UTC)
click-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2025 9:09pm

Copy link
Collaborator

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

Can you add isDeleted and isDisabled to the Table story props so reviewers can see the behavior more easily please?

Comment on lines +213 to +214
$isDeleted?: boolean;
$isDisabled?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

From a component interface perspective, do we need both of these properties? isDeleted seems kind of implementation specific. Couldn't isDisabled handle that? I'm not seeing isDeleted used on tables in CP.

Maybe @vineethasok or @gjones could answer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, I'd try to keep it as simple as possible until there is a clear reason why we need a separate style between isDeleted and isDisabled, and if we decide there's a legit reason, they should look distinct from one another.

@vineethasok
Copy link
Collaborator

@gjones Design wise do we need to add a new token for the disabled state for these cases?
Like the icon button and other form fields

Copy link

@crisalbu crisalbu left a comment

Choose a reason for hiding this comment

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

Hey @natalyjazzviolin , the colour should be the same as the colour for enabled columns

Screenshot 2025-03-13 at 18 10 05 Screenshot 2025-03-13 at 18 09 41

We need to make sure that for disabled/removed columns, the + action doesn't have the same colour as the pencil.

@natalyjazzviolin
Copy link
Contributor Author

@crisalbu do I understand correctly that if a row is disabled/removed, the pencil needs to be a muted color, and the x or + icon the same color as an enabled row would have?
In this PR I only changed the styling of the + icon case, not the x icon case.

${({ theme, $size }) => `
color: ${theme.click.table.row.color.text.default};
${({ theme, $size, $isDeleted, $isDisabled }) => `
color: ${$isDeleted || $isDisabled ? "tomato" : "blue"};
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does tomato and blue do here?

@gjones
Copy link
Collaborator

gjones commented Mar 15, 2025

@gjones Design wise do we need to add a new token for the disabled state for these cases?
Like the icon button and other form fields

@vineethasok are you thinking something like click.table.row.color.text.disabled?

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.

Table: row button is greyed out
5 participants