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(panel): Add component tokens #10519

Closed
wants to merge 12 commits into from
Closed

Conversation

macandcheese
Copy link
Contributor

@macandcheese macandcheese commented Oct 10, 2024

Related Issue: #7180

Expected test failures.
This will also serve as a pattern for consuming component like Dialog, Sheet, etc.

Summary

--calcite-panel-corner-radius: Specifies the corner radius of the component.
--calcite-panel-shadow: Specifies the shadow of the component.
--calcite-panel-heading-text-color: Specifies the text color of the component's heading.
--calcite-panel-description-text-color: Specifies the text color of the component's description.

--calcite-panel-background-color: Specifies the background color of the component.
--calcite-panel-header-background-color: Specifies the background color of the component's footer.
--calcite-panel-footer-background-color: Specifies the background color of the component's footer.
--calcite-panel-content-top-background-color: Specifies the border color of the "content-top" slot.
--calcite-panel-content-bottom-background-color: Specifies the border color of the "content-bottom" slot.

--calcite-panel-border-color: Specifies the border color of the component.

--calcite-panel-space: Specifies the padding of the component's "unnamed (default)" slot.
--calcite-panel-header-content-space: Specifies the padding of the "header-content" slot.
--calcite-panel-footer-space: Specifies the padding of the component's footer.
--calcite-panel-content-top-space: Specifies the padding of the "content-top" slot.
--calcite-panel-content-bottom-space: Specifies the padding of the "content-bottom" slot.

--calcite-panel-actions-background-color: Specifies the background color of Panel's closable, collapsible, and slotted and overflowing header-actions-end elements.
--calcite-panel-actions-background-color-hover: Specifies the background color of Panel's closable, collapsible, and slotted and overflowing header-actions-end elements when hovered.
--calcite-panel-actions-background-color-pressed: Specifies the background color of Panel's closable, collapsible, and slotted and overflowing header-actions-end elements when pressed.
--calcite-panel-actions-text-color: Specifies the text color of Panel's closable, collapsible, and slotted and overflowing header-actions-end elements.
--calcite-panel-actions-text-color-pressed: Specifies the text color of Panel's closable, collapsible, and slotted and overflowing header-actions-end elements when pressed.

--calcite-panel-content-space: [Deprecated] Specifies the padding of the component's content.
--calcite-panel-footer-padding: [Deprecated] Specifies the padding of the component's footer.
--calcite-panel-header-border-block-end: [Deprecated] Specifies the component header's block end border.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Oct 10, 2024
@macandcheese macandcheese added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 11, 2024
@macandcheese macandcheese marked this pull request as ready for review October 11, 2024 01:48
@macandcheese macandcheese 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 Oct 11, 2024
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Oct 21, 2024
@macandcheese macandcheese added pr ready for visual snapshots Adding this label will run visual snapshot testing. design-tokens Issues requiring design tokens. and removed Stale Issues or pull requests that have not had recent activity. pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 21, 2024
@macandcheese
Copy link
Contributor Author

macandcheese commented Oct 21, 2024

@alisonailea @jcfranco not sure why Chromatic isn't firing off (maybe because it was initially opened as draft?), but there is a local demo and it's ready for nomenclature review.

There are some expected test / Chromatic failures right now, but wanted to get eyes on the verbiage first. I'll add components like Dialog, etc., that render a Panel in this PR when tokens are settled.

@macandcheese macandcheese 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 Oct 21, 2024
Copy link
Contributor

@alisonailea alisonailea left a comment

Choose a reason for hiding this comment

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

Based on the wiki I think this can be simplified significantly. In most cases users can target the slotted content styles rather than adding another component token.

--calcite-panel-corner-radius: Specifies the corner radius of the component.
--calcite-panel-shadow: Specifies the shadow of the component.
--calcite-panel-heading-text-color: Specifies the text color of the component's `heading`.
--calcite-panel-text-color: Specifies the text color of the component.
--calcite-panel-background-color: Specifies the background color of the component.
--calcite-panel-header-background-color: Specifies the background color of the component's footer.
--calcite-panel-footer-background-color: Specifies the background color of the component's footer.
--calcite-panel-border-color: Specifies the border color of the component.
--calcite-panel-content-space: Specifies the padding of the component's content slots.
--calcite-panel-header-space: Specifies the padding of the component's header.
--calcite-panel-footer-space: Specifies the padding of the component's footer.
--calcite-panel-footer-padding: [Deprecated] Specifies the padding of the component's footer.
--calcite-panel-header-border-block-end: [Deprecated] Specifies the component header's block end border.

* @prop --calcite-panel-header-background-color: Specifies the background color of the component's footer.
* @prop --calcite-panel-footer-background-color: Specifies the background color of the component's footer.
* @prop --calcite-panel-content-top-background-color: Specifies the border color of the `"content-top"` slot.
* @prop --calcite-panel-content-bottom-background-color: Specifies the border color of the `"content-bottom"` slot.
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a request for a header and footer bg color but the content is already covered by the more general --calcite-panel-background-color and there is no request for unique background colors for top and bottom content. Also, I don't think we need to add extra tokens to cover slotted content.

  • remove --calcite-panel-content-top-background-color
  • remove --calcite-panel-content-bottom-background-color

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify - there is padding rendered around the slotted content. This means that a user cannot actually control the background, padding, or border color of these areas (and why Action Bar has no corresponding background color or padding props, because there is no UI surface a user can't control).

I don't understand why we'd add these for some of the content areas and not others - if a user wants to theme the header and footer it is a logical next request to theme these values.

Copy link
Contributor

Choose a reason for hiding this comment

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

tokens are meant to describe our design patterns. This does not represent our design patterns. If a user needs this and can provide a reasonable use-case for it then we can consider it but until then it's just adding extra bulk

* @prop --calcite-panel-footer-border-color: Specifies the border color of the component's footer.
* @prop --calcite-panel-content-top-border-color: Specifies the border color of the `"content-top"` slot.
* @prop --calcite-panel-content-bottom-border-color: Specifies the border color of the `"content-bottom"` slot.
* @prop --calcite-panel-action-bar-border-color: Specifies the border color of the `"action-bar"` slot.
Copy link
Contributor

Choose a reason for hiding this comment

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

there are no requests for unique border colors for different parts of the component and we should not be adding extra tokens to cover slotted content. A single border-color token is sufficient here.
remove

  • --calcite-panel-header-border-color
  • --calcite-panel-footer-border-color
  • --calcite-panel-content-top-border-color
  • --calcite-panel-content-bottom-border-color
  • --calcite-panel-action-bar-border-color

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 can simplify these into one - however an implementation q:

From a theming use case, I want to add a border color to the outside of the Panel alongside a shadow (Dashboard, Knowledge Studio, etc.). I've set up the "outside" shadow with a box-shadow, keying off the --calcite-panel-border-color. Do we want a "content-border" color that could apply to all 5, and the existing "border" color. That would simplify it to just two.

* @prop --calcite-panel-actions-background-color-hover: Specifies the background color of Panel's `closable`, `collapsible`, and slotted and overflowing `header-actions-end` elements when hovered.
* @prop --calcite-panel-actions-background-color-pressed: Specifies the background color of Panel's `closable`, `collapsible`, and slotted and overflowing `header-actions-end` elements when pressed.
* @prop --calcite-panel-actions-text-color: Specifies the text color of Panel's `closable`, `collapsible`, and slotted and overflowing `header-actions-end` elements.
* @prop --calcite-panel-actions-text-color-pressed: Specifies the text color of Panel's `closable`, `collapsible`, and slotted and overflowing `header-actions-end` elements when pressed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove all these styles for slotted content.

remove

  • --calcite-panel-actions-background-color
  • --calcite-panel-actions-background-color-hover
  • --calcite-panel-actions-background-color-pressed
  • --calcite-panel-actions-text-color
  • --calcite-panel-actions-text-color-pressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does a user change these? The component renders Action for closable, collapsible, and the header-actions-end renders its own Action Menu that is not user-slotted. Flow Item will render a back button, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

they can use the action component tokens

#close,
#collapse,
.back-button,
calcite-action-menu {
Copy link
Contributor

Choose a reason for hiding this comment

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

none of these tokens are necessary so we can just remove this style block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you clarify? These are all set on internally-rendered components that Panel provides, not user-slotted content. I guess we could fallback to the BG color of the Panel (or Header), but this doesn't provide the hover or press states needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

these are covered by button and action tokens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are internally rendered based on properties (or an auto-magic rendering of ... menu on behalf of the user):
Screenshot 2024-11-06 at 8 14 01 AM

The "unthemed" action one above is directly slotted and not affected by these properties - of course in those situations users should theme on the action itself.

Users should not have to guess or have any knowledge of what components we use internally. It could be an action today - it might be a div or a button tomorrow - that implementation detail should be hidden from the user and they should be able to set these on the component that renders these internals. Having to set the following to affect internally rendered components seems unexpected:

.my-themed-panel {
--calcite-panel-background-color: red;
--calcite-action-background-color: red;
}

Curious what others think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Users should not have to guess or have any knowledge of what components we use internally

in this case, yes they should. We should not add a ton of extra tokens when we can just document that --calcite-action- tokens are used by putting them in the jsdoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following that though, setting “—calcite-action-whatever” on the parent Panel will affect all slotted content anywhere in the component.

Panels may have dozens of slotted Actions not intended to be styled the same way a user would like to style the internally rendered “clearable” type elements.

So we’d have to doc a public css prop from another component on Panel that would unintentionally affect child components. This feels more confusing and likely frustrating for a user.

Copy link
Member

Choose a reason for hiding this comment

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

@alisonailea @macandcheese Can we take a step back and define what a simple custom theme will look like? I think this is a key piece of information that we're missing from the component token documentation. The documented approach won't scale for internal components since external element-scoped styling (like this example) won't apply within shadow DOM (especially overrides that depend on state pseudo-classes).

#collapse,
calcite-action-menu {
&:last-child {
--calcite-action-corner-radius-start-end: var(--calcite-panel-corner-radius);
Copy link
Contributor

Choose a reason for hiding this comment

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

--calcite-action-corner-radius-start-end token will be removed per our conversation about handling border radius on buttons. Instead we can just target the style itself.

#close,
#collapse,
calcite-action-menu {
  &:last-child {
    border-start-end-radius: var(--calcite-panel-corner-radius, var(--calcite-corner-radius);
  }
}

}

.back-button {
--calcite-action-corner-radius-start-start: var(--calcite-panel-corner-radius);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

#close,
.back-button {
    border-start-start-radius: var(--calcite-panel-corner-radius, var(--calcite-corner-radius);
  }
}

@@ -165,29 +229,30 @@
overflow-auto
h-full
focus-base;
padding: var(--calcite-panel-content-space, 0);
padding: var(--calcite-panel-space, var(--calcite-panel-content-space, 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think --calcite-panel-space is helpful here. Lets just style .content-top and .content-bottom with --calcite-panel-content-space

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 think these are still useful - there are many reasons why I'd want different spacing values between these - and because Dialog sets just the content padding to a non-zero default, having them separate ensures we can style each appropriately.

Copy link
Contributor

Choose a reason for hiding this comment

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

give me a specific use case?

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 want to put a progress bar in the content-top area and not have space around it. I want to put stepper items in the content-top area and not have space around them. I want to place a full-width banner, aggregate messaging, or notice in the content-top area and not have space around it. There are dozens of use cases. If you'd like, I can remove these changes from this PR, and address the request from here down the road: #10380

@macandcheese macandcheese 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 Oct 21, 2024
@macandcheese macandcheese 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 Oct 21, 2024
Copy link
Contributor

github-actions bot commented Nov 1, 2024

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Nov 1, 2024
@macandcheese macandcheese removed the Stale Issues or pull requests that have not had recent activity. label Nov 5, 2024
@macandcheese macandcheese 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 Nov 5, 2024
@macandcheese macandcheese 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 Nov 5, 2024
@macandcheese macandcheese 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 Nov 6, 2024
* @prop --calcite-panel-header-background-color: Specifies the background color of the component's footer.
* @prop --calcite-panel-footer-background-color: Specifies the background color of the component's footer.
* @prop --calcite-panel-content-top-background-color: Specifies the border color of the `"content-top"` slot.
* @prop --calcite-panel-content-bottom-background-color: Specifies the border color of the `"content-bottom"` slot.
Copy link
Contributor

Choose a reason for hiding this comment

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

tokens are meant to describe our design patterns. This does not represent our design patterns. If a user needs this and can provide a reasonable use-case for it then we can consider it but until then it's just adding extra bulk

* @prop --calcite-panel-actions-background-color-hover: Specifies the background color of Panel's `closable`, `collapsible`, and slotted and overflowing `header-actions-end` elements when hovered.
* @prop --calcite-panel-actions-background-color-pressed: Specifies the background color of Panel's `closable`, `collapsible`, and slotted and overflowing `header-actions-end` elements when pressed.
* @prop --calcite-panel-actions-text-color: Specifies the text color of Panel's `closable`, `collapsible`, and slotted and overflowing `header-actions-end` elements.
* @prop --calcite-panel-actions-text-color-pressed: Specifies the text color of Panel's `closable`, `collapsible`, and slotted and overflowing `header-actions-end` elements when pressed.
Copy link
Contributor

Choose a reason for hiding this comment

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

they can use the action component tokens

#close,
#collapse,
.back-button,
calcite-action-menu {
Copy link
Contributor

Choose a reason for hiding this comment

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

these are covered by button and action tokens.

@@ -165,29 +229,30 @@
overflow-auto
h-full
focus-base;
padding: var(--calcite-panel-content-space, 0);
padding: var(--calcite-panel-space, var(--calcite-panel-content-space, 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

give me a specific use case?

@macandcheese macandcheese 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 Nov 20, 2024
@macandcheese
Copy link
Contributor Author

To avoid Stencil / Lumina merge conflicts, I've opened a fresh Lumina-based PR for this one: #10822

@macandcheese macandcheese deleted the macandcheese/panel-tokens branch November 21, 2024 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-tokens Issues requiring design tokens. 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.

4 participants