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.

Sign up for GitHub

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

Jump to bottom

TreeView left margin override support #2338

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

TreeView left margin override support #2338

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d55ef87
tree item padding from 10px to 16px (standardPadding)
fredvisser Aug 8, 2024
e4e193f
Change files
fredvisser Aug 8, 2024
22a1cfc
change token and shift all content
fredvisser Aug 8, 2024
139a35f
update anchor padding
fredvisser Aug 9, 2024
f79d82d
tree-view has 4px padding already; 16-4=12px
fredvisser Aug 9, 2024
1f8e120
Merge branch 'main' into users/fvisser/16px-tree-margin
fredvisser Aug 9, 2024
c93dc09
Merge branch 'main' into users/fvisser/16px-tree-margin
fredvisser Aug 13, 2024
9d888b6
review feedback
fredvisser Aug 13, 2024
c9cc5cb
update anchor-tree-item as well
fredvisser Aug 13, 2024
7b0d8ac
Update token name and fix MDX link formatting
fredvisser Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@ni-nimble-components-fdc2a0d0-af90-4afa-a47e-334c918f8566.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
Copy link
Member

@rajsite rajsite Aug 12, 2024 •
edited
Loading

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:
image

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.

"type": "patch",
fredvisser marked this conversation as resolved.
Show resolved Hide resolved
"comment": "Update tree item padding from 10px to 8px (mediumPadding) to match standard layout requirements, and expose `treeViewLeftMargin` token to allow override for specific use cases.",
"packageName": "@ni/nimble-components",
"email": "[email protected]",
"dependentChangeType": "patch"
}
5 changes: 3 additions & 2 deletions packages/nimble-components/src/anchor-tree-item/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ import {
fillSelectedColor,
bodyFontSize,
bodyDisabledFontColor,
iconColor
iconColor,
treeViewLeftMargin
} from '../theme-provider/design-tokens';
import { focusVisible } from '../utilities/style/focus';
import { userSelectNone } from '../utilities/style/user-select';
Expand Down Expand Up @@ -86,7 +87,7 @@ export const styles = css`
align-items: center;
white-space: nowrap;
width: 100%;
padding-left: 10px;
padding-left: ${treeViewLeftMargin};
font: inherit;
font-size: ${bodyFontSize};
${userSelectNone}
Expand Down
1 change: 1 addition & 0 deletions packages/nimble-components/src/theme-provider/design-token-comments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export const comments: { readonly [key in TokenName]: string } = {
dialogLargeMaxHeight: 'Standard maximum height for large dialogs.',
menuMinWidth: 'Standard menu min width for menu popup.',
bannerGapSize: 'Space between stacked banners',
treeViewLeftMargin: 'Left margin for tree view items',
spinnerSmallHeight: 'Small height (16px) for a spinner component',
spinnerMediumHeight: 'Medium height (32px) for a spinner component',
spinnerLargeHeight: 'Large height (64px) for a spinner component',
Expand Down
1 change: 1 addition & 0 deletions packages/nimble-components/src/theme-provider/design-token-names.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export const tokenNames: { readonly [key in TokenName]: string } = {
dialogLargeWidth: 'dialog-large-width',
dialogLargeHeight: 'dialog-large-height',
dialogLargeMaxHeight: 'dialog-large-max-height',
treeViewLeftMargin: 'tree-view-left-margin',
fredvisser marked this conversation as resolved.
Show resolved Hide resolved
menuMinWidth: 'menu-min-width',
bannerGapSize: 'banner-gap-size',
spinnerSmallHeight: 'spinner-small-height',
Expand Down
3 changes: 3 additions & 0 deletions packages/nimble-components/src/theme-provider/design-tokens.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,9 @@ export const menuMinWidth = DesignToken.create<string>(
export const bannerGapSize = DesignToken.create<string>(
styleNameFromTokenName(tokenNames.bannerGapSize)
).withDefault('1px');
export const treeViewLeftMargin = DesignToken.create<string>(
styleNameFromTokenName(tokenNames.treeViewLeftMargin)
).withDefault(mediumPadding);

export const spinnerSmallHeight = DesignToken.create<string>(
styleNameFromTokenName(tokenNames.spinnerSmallHeight)
Expand Down
7 changes: 4 additions & 3 deletions packages/nimble-components/src/tree-item/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ import {
iconSize,
mediumDelay,
bodyDisabledFontColor,
iconColor
iconColor,
treeViewLeftMargin
} from '../theme-provider/design-tokens';
import { groupSelectedAttribute } from '../tree-view/types';
import { DirectionalStyleSheetBehavior } from '../utilities/style/direction';
Expand Down Expand Up @@ -98,7 +99,7 @@ export const styles = css`
align-items: center;
white-space: nowrap;
width: 100%;
padding-left: 10px;
padding-left: ${treeViewLeftMargin};
font: inherit;
font-size: ${bodyFontSize};
${userSelectNone}
Expand All @@ -119,7 +120,7 @@ export const styles = css`
padding: 0px;
justify-content: center;
align-items: center;
margin-left: 10px;
margin-left: ${treeViewLeftMargin};
position: absolute;
}

Expand Down
16 changes: 14 additions & 2 deletions packages/storybook/src/nimble/tree-view/tree-view.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { html, repeat, when } from '@microsoft/fast-element';
import { withActions } from '@storybook/addon-actions/decorator';
import type { HtmlRenderer, Meta, StoryObj } from '@storybook/html';
import { treeViewLeftMargin } from '../../../../nimble-components/src/theme-provider/design-tokens';
import { iconCogTag } from '../../../../nimble-components/src/icons/cog';
import { iconDatabaseTag } from '../../../../nimble-components/src/icons/database';
import { treeItemTag } from '../../../../nimble-components/src/tree-item';
Expand All @@ -21,6 +22,7 @@ interface TreeArgs {
options: ItemArgs[];
expandedChange: undefined;
selectedChange: undefined;
leftMargin: string;
}

interface ItemArgs {
Expand Down Expand Up @@ -199,11 +201,20 @@ export const multipleTreeItems: StoryObj<TreeArgs> = {
description:
'Event emitted when an item is selected or deselected.',
table: { category: apiCategory.events }
},
leftMargin: {
name: 'Left margin',
description: 'Overrides the default left margin of the tree view.',
table: { category: apiCategory.styles },
control: { type: 'text' }
Comment on lines +205 to +209
Copy link
Member

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. )

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 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?

Copy link
Member

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.

Copy link
Member

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

}
},
// prettier-ignore
render: createUserSelectedThemeStory(html`
<${treeViewTag} selection-mode="${x => x.selectionMode}">
<${treeViewTag}
selection-mode="${x => x.selectionMode}"
style="${x => `${treeViewLeftMargin.cssCustomProperty}:${x.leftMargin};`}"
>
${repeat(x => x.options, html<ItemArgs>`
<${treeItemTag} ?expanded="${x => x.expanded}" value="${x => x.value}">
${when(x => x.icon, html`<${iconDatabaseTag} slot="start"></${iconDatabaseTag}>`)}
Expand Down Expand Up @@ -263,6 +274,7 @@ export const multipleTreeItems: StoryObj<TreeArgs> = {
selected: false,
expanded: false
}
]
],
leftMargin: '8px'
}
};
Loading