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(button, fab): add component tokens #8922

Merged
merged 18 commits into from
Mar 13, 2024

Conversation

alisonailea
Copy link
Contributor

Related Issue: #7180

Summary

Button

--calcite-button-background-color: defines the component's background color.
--calcite-button-border-color: defines the component's border color.
--calcite-button-corner-radius: defines the component's corner radius.
--calcite-button-loader-color: defines the component's loader color.
--calcite-button-shadow: defines the component's shadow.
--calcite-button-text-color: defines the component's text color.
--calcite-button-icon-color: defines the color of an icon in the component.

FAB

--calcite-fab-background-color: defines the component's background color.
--calcite-fab-border-color: defines the component's border color.
--calcite-fab-corner-radius: defines the component's corner radius.
--calcite-fab-loader-color: defines the component's loader color.
--calcite-fab-shadow: defines the component's shadow.
--calcite-fab-text-color: defines the component's text color.
--calcite-fab-icon-color: defines the color of an icon in the component.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Mar 9, 2024
@alisonailea alisonailea added the enhancement Issues tied to a new feature or request. label Mar 9, 2024
@alisonailea alisonailea changed the base branch from main to epic/7180-component-tokens March 9, 2024 02:41
@alisonailea alisonailea force-pushed the astump/7180-button-fab branch from be9e669 to 832ad82 Compare March 9, 2024 03:01
@alisonailea alisonailea added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Mar 9, 2024
@alisonailea alisonailea added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Mar 12, 2024
@alisonailea alisonailea removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Mar 13, 2024
@alisonailea alisonailea added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Mar 13, 2024
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Just had one comment about a possible missing CSS prop on tab-nav. Other than that, this looks great! 🚀

@@ -3,6 +3,18 @@
*
* These properties can be overridden using the component's tag as selector.
*
* @prop --calcite-color-picker-save-buttons-background-color-active: defines the background color of the button when in a active state in the component.
Copy link
Member

Choose a reason for hiding this comment

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

Not critical for this PR since it can be tackled when color-picker gets updated, but save-buttons in the name could be misleading. Is there another name we could use to describe the saved color section, which contains both add/remove color buttons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I previously had it named --calcite-color-picker-button-background-color-active but was told that sounded to generic but I think that might be the right option here since add/remove are the only buttons in the component.

* @prop --calcite-tab-nav-button-icon-color: Specifies the color of the scroll buttons' icon.
* @prop --calcite-tab-nav-button-background-color-active: defines the scroll button's background color in an active state.
* @prop --calcite-tab-nav-button-background-color-hover: defines the scroll button's background color in a hover state.
* @prop --calcite-tab-nav-button-background-color: defines the scroll button's background color.
Copy link
Member

Choose a reason for hiding this comment

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

Does this need --calcite-tab-nav-button-background-color-focus?

@@ -5,6 +5,12 @@
*
* @prop --calcite-inline-editable-background-color: Specifies the background color of the component.
* @prop --calcite-inline-editable-border-color: Specifies the border color of the component.
* @prop --calcite-inline-editable-button-corner-radius: defines the button's corner radius.
Copy link
Member

@jcfranco jcfranco Mar 13, 2024

Choose a reason for hiding this comment

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

This one isn't critical either as it can be addressed in #8888, but I think we need 3 different sets of button props for this one:

  1. edit affordance
  2. confirm/cancel buttons

Each one of the above button instances is styled differently. Here's how I've got them currently set up.

@alisonailea alisonailea added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Mar 13, 2024
@alisonailea alisonailea added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Mar 13, 2024
@alisonailea alisonailea merged commit 0f7ae82 into epic/7180-component-tokens Mar 13, 2024
8 checks passed
@alisonailea alisonailea deleted the astump/7180-button-fab branch March 13, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants