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

Add nimble token for toolbar height #2493

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+16 −2
Open

Add nimble token for toolbar height #2493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df4a10d
Add toobar standard height token
aagash-ni Dec 11, 2024
5de8c34
Change files
aagash-ni Dec 11, 2024
231085c
Lint fix
aagash-ni Dec 11, 2024
5b12d05
Update change type
aagash-ni Dec 11, 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-a9107da1-a4c4-492b-9842-3d65680f0a8c.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for thinking of trying to single-source this in Nimble. But I have an initial concern: the Nimble toolbar itself would not use this token, so it seems strange to expose it as the "official" toolbar height. My concern with that is that a client would probably expect that they could override the value of this token and have all Nimble toolbars update, but that would not be the case.

The reason the Nimble toolbar wouldn't use this token is that the toolbar sizes itself based on the height of its content plus some vertical padding (32px +2*4px = 40px). I suspect we want to keep that behavior because we may want both shorter and taller toolbars for certain use cases.

Could the use cases you describe follow the same approach and avoid the need for a new token entirely? A downside of that approach is that if we ever change our minds about this behavior (e.g. change the vertical padding) then it would be hard to identify all the places that need to change. But I personally think that's an acceptable risk.

I'm going to mark this as not approved for now but I'm happy to continue the discussion. However, me and the other Nimble reviewers are all going to be OOO for the rest of the year so if you want to continue the conversation it'll have to wait for next year. In the meantime hopefully you can get by with duplicating this value in a few places.

"type": "minor",
"comment": "Add nimble token for toolbar height",
"packageName": "@ni/nimble-components",
"email": "[email protected]",
"dependentChangeType": "patch"
}
3 changes: 2 additions & 1 deletion 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 @@ -322,5 +322,6 @@ export const comments: { readonly [key in TokenName]: string } = {
'Background fill color for the editable calendar event grab handle',
calendarGridBorderColor: 'Border color for the calendar grid',
calendarGroupHeaderBackgroundColor:
'Background color for the calendar resource group header'
'Background color for the calendar resource group header',
toolbarHeight: 'Standard height for the toolbar'
};
4 changes: 3 additions & 1 deletion 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 @@ -263,7 +263,9 @@ export const tokenNames: { readonly [key in TokenName]: string } = {
calendarEventFillBlockedColor: 'calendar-event-fill-blocked-color',
calendarGrabHandleBackgroundColor: 'calendar-grab-handle-background-color',
calendarGridBorderColor: 'calendar-grid-border-color',
calendarGroupHeaderBackgroundColor: 'calendar-group-header-background-color'
calendarGroupHeaderBackgroundColor:
'calendar-group-header-background-color',
toolbarHeight: 'toolbar-height'
};

const prefix = 'ni-nimble';
Expand Down
4 changes: 4 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 @@ -520,6 +520,10 @@ export const spinnerLargeHeight = DesignToken.create<string>(
styleNameFromTokenName(tokenNames.spinnerLargeHeight)
).withDefault('64px');

export const toolbarHeight = DesignToken.create<string>(
styleNameFromTokenName(tokenNames.toolbarHeight)
).withDefault('40px');

// The token gets a default value of the table's default height (480px)
// but is given a calculated value in the table styles.
export const tableFitRowsHeight = DesignToken.create<string>(
Expand Down
Loading