-
Notifications
You must be signed in to change notification settings - Fork 201
fix(treeview): update Treeview margin and focus border to use Spectrum tokens #3257
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
base: main
Are you sure you want to change the base?
Conversation
|
c68f4e3
to
d2272ea
Compare
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.
Hello! I really appreciate this contribution, however, due to some of the rules in our system about when to use certain tokens, I'm not sure if we'll be able to accept this change at this time. Please don't hesitate to reach out with questions!
@@ -28,7 +28,7 @@ | |||
--spectrum-treeview-heading-color: var(--spectrum-heading-color); | |||
|
|||
--spectrum-treeview-item-border-size: var(--spectrum-border-width-200); | |||
--spectrum-treeview-item-border-size-selected: 1px; | |||
--spectrum-treeview-item-border-size-selected: var(--spectrum-divider-thickness-small); |
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.
Though this resolves to 1px, we don't mix component-level tokens like this in our system. We'd need to have a --spectrum-tree-view-divider-thickness
value (or something of the like) added to the token data in order to update this.
@@ -13,7 +13,7 @@ | |||
|
|||
.spectrum-TreeView { | |||
--spectrum-treeview-line-height: var(--spectrum-line-height-200); | |||
--spectrum-treeview-margin-block: 1em; | |||
--spectrum-treeview-margin-block: var(--spectrum-spacing-300);; |
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.
Thanks for tackling this! Unfortunately, the spacing-300 value does not resolve to the same rendered result as 1em
since the ems are relative values to the typography. This was an intentional choice made for tree view to ensure spacing mirrors the sizing of the content.
Description
No specific issue for this change. While working on a Treeview implementation, I noticed two lines of the CSS where we could use Spectrum tokens.
How and where has this been tested?
Tested locally by:
spectrum-css
Storybook locallyValidation steps
Validate visual Tree View before and after, specifically for margin and focus border visual differences. They should be identical.
Regression testing
Validate:
Screenshots
To-do list