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(split-button, dropdown-item, button): update component tokens #9456

Merged

Conversation

alisonailea
Copy link
Contributor

@alisonailea alisonailea commented May 29, 2024

Related Issue: #7180

Summary

Add theme e2e tests and update tokens
In order to get proper styling of split button this required some updates to dropdown-item and button

@@ -6,6 +6,10 @@
* @prop --calcite-button-background-color: defines the component's background color.
* @prop --calcite-button-border-color: defines the component's border color.
* @prop --calcite-button-corner-radius: defines the component's corner radius.
* @prop --calcite-button-corner-radius-start-start: defines the component's start-start corner radius. Will fallback to --calcite-button-corner-radius if defined.
Copy link
Member

Choose a reason for hiding this comment

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

Should this, and similar lines, say if not defined vs if defined?

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 was thinking about this. I'm not sure what makes more sense. It will only fallback to the more general -corner-radius var if that var is defined. Which is why I used if defined but maybe that's implied?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these just so Split Button can pass some through? Should these be made internal if so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are these just so Split Button can pass some through? Should these be made internal if so?

I thought about that. My reasoning for making them public was that customers may be using calcite-buttons to create their own components with similar edge case requirements so providing the option to set the corners separately would be beneficial

& calcite-icon {
color: theme("backgroundColor.brand");
color: var(--calcite-dropdown-item-icon-color, theme("backgroundColor.brand"));
Copy link
Member

Choose a reason for hiding this comment

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

Should this be setting --calcite-icon-color?

@@ -14,28 +172,134 @@
@apply w-full;
}

//--calcite-internal-split-button-loader-color: var(--calcite-color-inverse);
Copy link
Member

Choose a reason for hiding this comment

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

Commented out line should be removed. Applies to other commented-out lines.

If any commented-out line is waiting on a decision or question, let's add context and keep the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the catch

@alisonailea alisonailea merged commit 8f87921 into epic/7180-component-tokens May 30, 2024
4 checks passed
@alisonailea alisonailea deleted the astump/7180-split-button-dropdown branch May 30, 2024 21:51
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.

3 participants