-
Notifications
You must be signed in to change notification settings - Fork 8
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
TreeView left margin override support #2338
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,7 @@ | |||
{ |
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.
To summarize the concerns from Friday, the direction I think we should go is that the tree in nimble uses more normal padding values. If we look at a tree view expander aligned above a normal icon button you see the following:
You can see the left-padding on the tree-view is too big (10px). Normally we would expect 8px left spacing-16px icon-8px spacing. So the issue with the nimble tree-view is that padding-left should be 8px and not 10 or 12 px.
The second step is that the systemlink header seems to be using a non-standard icon button with custom sizing. It does not have a square 32px by 32px shape and seems to be a one-off unique button. The tree-view in nimble should not be adjusted to the sizing of that unique button only used in the sl-header.
Instead if we need to override this padding for one unique use-case then it should just be done in the sl-header styles or at most we create a design token that can be configured externally.
packages/nimble-components/src/theme-provider/design-token-names.ts
Outdated
Show resolved
Hide resolved
leftMargin: { | ||
name: 'Left margin', | ||
description: 'Overrides the default left margin of the tree view.', | ||
table: { category: apiCategory.styles }, | ||
control: { type: 'text' } |
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 having the token for configuration of this property is the right move but I don't know about broadcasting this so openly in the args table.
We could call it out in usage guidance in the mdx for the very specific use case. And we should have a test for the behavior. I'm actually tempted to have it as part of the matrix test to make sure it is interacting well with all the other modes that impact the spacing there (nested menu-items, icons, 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.
How about a compromise where we remove the docs from the args table and rely on the SWIF Chromatic tests to catch any regressions, instead of adding this to the matrix tests?
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'd like nimble features to have tests in nimble. I wouldn't expect adding a new state to the matrix test to be difficult.
If don't want to bloat the current test matrix can split the dimension out into a separate test pretty easily.
If don't think it's worth testing against the full matrix can make a separate matrix test for the relevant permutations.
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.
Moving the PR back to draft until we decide on the testing direction
Pull Request
π€¨ Rationale
The "sl-details-panel spacing tweaks" AzDO PR highlights the need to have icons start 16px from the edge, to bring all elements into alignment.
π©βπ» Implementation
tree-item
andanchor-tree-item
to change the content-regionleft-padding
from 10px to mediumPadding (8px) to align with standard layout grids.treeViewPaddingLeftSize
token, so that this value can be overridden by client apps that need a larger marginπ§ͺ Testing
β Checklist