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

refactor: Use component tokens within includes #8771

Merged
merged 22 commits into from
Feb 28, 2024

Conversation

driskull
Copy link
Member

@driskull driskull commented Feb 16, 2024

Related Issue: #7180

Summary

  • Uses component tokens within include files.
  • Removes unused spacing file.
  • Moves xButton mixin to its own file
  • Converts header to a mixin
  • removes animation import from some components (it only has mixins and functions)
  • Further issues should be created to fix any components using heading or xButton
  • Removes unused global vars

@driskull driskull marked this pull request as ready for review February 16, 2024 20:16
@driskull driskull requested a review from a team as a code owner February 16, 2024 20:16
@alisonailea
Copy link
Contributor

If we're going to keep using these we should convert them to @mixins with passable parameters so each component can use it's own variables.

@driskull
Copy link
Member Author

driskull commented Feb 17, 2024

If we're going to keep using these we should convert them to @Mixins with passable parameters so each component can use it's own variables.

Agreed. This could be done in any one of the following components for #7180 or in a follow up issue.

  • block
  • pick-list-group
  • tip
  • tip-manager

Combobox is the only component that uses xButton so it really shouldn't be in the includes. Ill move it to its own file.

@driskull
Copy link
Member Author

driskull commented Feb 17, 2024

@jcfranco @macandcheese whats the history of x-button? Shouldn't we just use a calcite-action or calcite-button instead? Not sure we need the xButton functional component.

Functional components should have their own CSS file as well. includes should be for more generic mixins.

@macandcheese
Copy link
Contributor

I think it was built to eventually support all close affordances - accepting arguments for round, square, stretch to fill, etc. as needed, but seems like it hasn't been adopted much.

@alisonailea
Copy link
Contributor

Input could also be a mixin, given there are so many very similar components

@driskull driskull requested a review from alisonailea February 20, 2024 22:21
# Conflicts:
#	packages/calcite-components/src/components/progress/progress.scss
@driskull driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Feb 22, 2024
@driskull driskull removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Feb 22, 2024
@driskull driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Feb 22, 2024
@driskull driskull 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 Feb 22, 2024
@driskull driskull 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 Feb 22, 2024
@driskull driskull 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 Feb 28, 2024
@driskull driskull 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 Feb 28, 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.

🕹️👾

margin: 0px;
align-self: center;
color: var(#{$textColor}, var(--calcite-color-text-3));
transition:
Copy link
Member

Choose a reason for hiding this comment

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

Could you explicitly list the props that will be transitioned by state? Looks like it's color and background-color.

$borderWidth: "--calcite-border-width-md"
) {
.x-button {
appearance: none;
Copy link
Member

Choose a reason for hiding this comment

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

I thought we agreed to only inline Tailwind utils related to component tokens. FWIW, we can automate this step later to make it easier for review.

@driskull driskull 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 Feb 28, 2024
@driskull driskull merged commit 1393c1d into epic/7180-component-tokens Feb 28, 2024
8 checks passed
@driskull driskull deleted the dris0000/7180-mixins branch February 28, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants