Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(panel): Add component tokens #10519
Changes from 4 commits
3fe7966
f2b3623
75edad3
18d69b8
dddb2b2
257df60
3d890f5
c426ed3
6a61ba3
dce1b5a
00bb74d
4be06d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 theheader-actions-end
renders its own Action Menu that is not user-slotted. Flow Item will render aback
button, etc.There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just remove bgcolor here and let it inherit from the parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can just be --calcite-panel-content-space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--calcite-panel-content-space
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
![Screenshot 2024-11-06 at 8 14 01 AM](https://private-user-images.githubusercontent.com/4733155/383638414-68845849-d044-400f-ba0b-ed5f13b2b250.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk1ODQwOTAsIm5iZiI6MTczOTU4Mzc5MCwicGF0aCI6Ii80NzMzMTU1LzM4MzYzODQxNC02ODg0NTg0OS1kMDQ0LTQwMGYtYmEwYi1lZDVmMTNiMmIyNTAucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxNSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTVUMDE0MzEwWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MzU2YjFkMWMwNzMxZjFmMTJhYjY4MTI5ZmNhZTdlMjYzYTVjZjRlM2Y3NDExMjkxMWMyM2U1NDUzNjdkNjAxZSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ._Yb93755oslMt4muzAHly8DLkVm19lBp9YHbcQnHB8Y)
...
menu on behalf of the user):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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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