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(accordion-item): style accordion item header in expanded state #10181

Merged
merged 9 commits into from
Sep 13, 2024

Conversation

alisonailea
Copy link
Contributor

@alisonailea alisonailea commented Aug 28, 2024

Related Issue: #7180 #4060

Summary

While this does not create additional tokens for the background color in a -hover or -press state explicitly because this is outside of our current design patterns, it does allow a user to accomplish this by only targeting the top-level component styles.

calcite-accordion-item:hover {
--calcite-accordion-item-background-color: blue;
}
calcite-accordion-item[expanded] {
--calcite-accordion-item-header-background-color: blue;
}

Tokens

--calcite-accordion-item-background-color: Specifies the component's background color.
--calcite-accordion-item-border-color: Specifies the component's border color.
--calcite-accordion-item-content-space: Specifies the component's padding.
--calcite-accordion-item-header-background-color: Specifies the background color of the component's header.
--calcite-accordion-item-text-color: Specifies the component's text color.
--calcite-accordion-item-heading-text-color: Specifies the component's heading text color.
--calcite-accordion-item-icon-color: Specifies the component's default icon color.
--calcite-accordion-item-start-icon-color: Specifies the component's start icon color. Fallback to --calcite-accordion-item-icon-color or current color.
--calcite-accordion-item-end-icon-color: Specifies the component's end icon color. Fallback to --calcite-accordion-item-icon-color or current color.
--calcite-accordion-item-expand-icon-color: Specifies the component's expand icon color.

Deprecated

--calcite-accordion-border-color: Use --calcite-accordion-item-border-color. Specifies the component's border color.
--calcite-accordion-text-color-hover: Use --calcite-accordion-item-text-color-hover. Specifies the component's main text color on hover.
--calcite-accordion-text-color-pressed: Use --calcite-accordion-item-text-color-press. Specifies the component's main text color when pressed.
--calcite-accordion-text-color: Use --calcite-accordion-item-text-color. Specifies the component's text color.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Aug 28, 2024
@alisonailea alisonailea marked this pull request as ready for review August 29, 2024 15:50
Copy link
Contributor

@macandcheese macandcheese left a comment

Choose a reason for hiding this comment

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

Looks good! Just one comment on text colors

* @prop --calcite-accordion-item-header-background-color: Specifies the background color of the component's header.
* @prop --calcite-accordion-item-text-color-hover: Specifies the component's main text color on hover.
* @prop --calcite-accordion-item-text-color-press: Specifies the component's main text color when pressed.
* @prop --calcite-accordion-item-text-color: Specifies the component's text color.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need two separate ones here for heading / description (and then apply whichever matches icon to icon) - They seem to use different values here? https://www.figma.com/design/vB6xFCAFeW0zKrT4nKlh8Z/Calcite-UI-Kit-%26-Styles?m=auto&node-id=3921-13&t=GRKfhHOwKkhpaNZS-1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The design pattern followed the -default, -hover, -press pattern, if we were to set tokens for heading / description we would be tripling the required tokens and breaking the design pattern.

Choose a reason for hiding this comment

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

All of our components with a heading and a description will use two different colors for the text. Accordion Item currently functions differently than the rest in that it has color changes onHover.

At rest:

  • heading is text.2
  • description + icon are text.3

onHover:

  • heading is text.1
  • description + icon are text.2
  • weird animation timings are present here: the icon and the description "fade" in at different rates while the heading doesn't not

When expanded (matches onHover):

  • heading is text.1
  • description + icon are text.2
  • no hover state when expanded

No other components with headings and descriptions behave this way and in issue #10016 (and epic #9299) we've outlined specs to consolidate these patterns. Even with the updated pattern though, the heading and description will be different colors, but they won't change on hover/press.

Since they use different colors I'd expect separate props for them. As for the hover/pressed tokens, I'm less sure since we're in the process of consolidating and aligning some of these different patterns across the system. If it's important to define these tokens for the current state, then I suppose they could be deprecated later when the pattern is updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So here's what I've done.

I have left the default styling as-is so that

At rest:
heading is text.2
description + icon are text.3

onHover:
heading is text.1
description + icon are text.2
weird animation timings are present here: the icon and the description "fade" in at different rates while the heading doesn't not

When expanded (matches onHover):
heading is text.1
description + icon are text.2
no hover state when expanded

HOWEVER, the component tokens available to the user reflect the future pattern where-in there is no extra hover/press state tokens.

Choose a reason for hiding this comment

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

Dig it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming:

Should this have --calcite-accordion-item-description-text-color in addition to --calcite-accordion-item-heading-text-color?

In this set up - a user can override the internally rendered icons, but only through --calcite-icon-color, nothing scoped to Accordion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--calcite-accordion-item-text-color == --calcite-accordion-item-description-text-color

@alisonailea alisonailea added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Sep 12, 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 Sep 13, 2024
@alisonailea alisonailea merged commit ca932b5 into dev Sep 13, 2024
13 checks passed
@alisonailea alisonailea deleted the astump/7180-accordion-item branch September 13, 2024 18:33
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