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

feat!: Add icon button #1021

Merged
merged 17 commits into from
Jan 10, 2024
Merged

feat!: Add icon button #1021

merged 17 commits into from
Jan 10, 2024

Conversation

alimpens
Copy link
Contributor

@alimpens alimpens commented Jan 5, 2024

Some questions:

  • Do we want to offer a disabled variant? An Icon Button usually doesn't have any context / text, which makes a disabled button even worse than usual for a11y. -> Discussed, a disabled Icon Button could be useful for application menu bar type things.
  • Is onBackground dark / light the right API? We also use onBackground dark for the Search Bar, which isn't necessarily on a dark background. -> We'll discuss this in a separate ticket
  • The Alert component should use the Icon Button as well. I didn't do that in this PR to prevent conflict with this PR
  • The component currently differs from the design, because having a default padding is a bit cumbersome in use. This padding is necessary for the SearchField component though, so we don't use IconButton there (yet). Discuss this with design.

@github-actions github-actions bot temporarily deployed to demo-DES-548-add-icon-button January 5, 2024 13:42 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-548-add-icon-button January 5, 2024 13:46 Destroyed
Copy link
Contributor

@VincentSmedinga VincentSmedinga left a comment

Choose a reason for hiding this comment

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

Nice cleanup in the other components!

@VincentSmedinga
Copy link
Contributor

Do we want to offer a disabled variant? An Icon Button usually doesn't have any context / text, which makes a disabled button even worse than usual for a11y.

We should strongly advise against disabling buttons in general, but I don’t think we can leave them out. I expect some disabled icon buttons in applications, e.g. to add or remove something an item from a set.

Is onBackground dark / light the right API? We also use onBackground dark for the Search Bar, which isn't necessarily on a dark background.

Let‘s explore this separately. If we keep using it, we should make clear which backgrounds are light and which are dark. I’ve left some comments on this with the code as well. We use it in other places; no need to hold back this PR.

The Alert component should use the Icon Button as well. I didn't do that in this PR to prevent conflict with this PR

Yes, I’ll make a task to remember that.

Co-authored-by: Vincent Smedinga <[email protected]>
@github-actions github-actions bot temporarily deployed to demo-DES-548-add-icon-button January 8, 2024 16:25 Destroyed
Co-authored-by: Vincent Smedinga <[email protected]>
@github-actions github-actions bot temporarily deployed to demo-DES-548-add-icon-button January 9, 2024 08:49 Destroyed
Co-authored-by: Vincent Smedinga <[email protected]>
@github-actions github-actions bot temporarily deployed to demo-DES-548-add-icon-button January 9, 2024 08:52 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-548-add-icon-button January 9, 2024 08:56 Destroyed
@alimpens
Copy link
Contributor Author

alimpens commented Jan 9, 2024

Do we want to offer a disabled variant? An Icon Button usually doesn't have any context / text, which makes a disabled button even worse than usual for a11y.

We should strongly advise against disabling buttons in general, but I don’t think we can leave them out. I expect some disabled icon buttons in applications, e.g. to add or remove something an item from a set.

Good point, I'll add it!

Is onBackground dark / light the right API? We also use onBackground dark for the Search Bar, which isn't necessarily on a dark background.

Let‘s explore this separately. If we keep using it, we should make clear which backgrounds are light and which are dark. I’ve left some comments on this with the code as well. We use it in other places; no need to hold back this PR.

Agreed, I've made a ticket for this

The Alert component should use the Icon Button as well. I didn't do that in this PR to prevent conflict with this PR

Yes, I’ll make a task to remember that.

@github-actions github-actions bot temporarily deployed to demo-DES-548-add-icon-button January 10, 2024 08:27 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-548-add-icon-button January 10, 2024 08:51 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-548-add-icon-button January 10, 2024 10:13 Destroyed
@VincentSmedinga VincentSmedinga merged commit b6e1c26 into develop Jan 10, 2024
6 checks passed
@VincentSmedinga VincentSmedinga deleted the feat/DES-548-add-icon-button branch January 10, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants