ni / nimble

The NI Nimble Design System
https://nimble.ni.dev
MIT License
32 stars 8 forks source link

Configurable / Additional heights for nimble controls #610

Open rajsite opened 2 years ago

rajsite commented 2 years ago

🧹 Tech Debt / Enhancement

Currently the height of nimble controls is defined by the --ni-nimble-control-height css custom property. Our control styles are written in such a way that they are dependent on --ni-nimble-control-height custom property such as with fixed css calc() statements that use the --ni-nimble-control-height property. This is further discussed in the "Arbitrary Sizing" section.

In addition, we currently do not support most of the additional component heights shown in Figma Specs. This is further discussed in the "Predesigned Sizing" section.

Arbitrary Sizing

The design of the height of nimble controls being defined by the '--ni-nimble-control-height` css custom property may be unexpected for users as, for example, a user may expect to set the height via css of a control:

<nimble-some-control style="height:50px"></nimble-some-control>

However, this may result in issues as by manually changing the height of the control, the --ni-nimble-control-height constant will not update in response and layout calculations using those values will layout incorrectly.

If a user really required changing the height of a control they have to do it via the --ni-nimble-control-height property:

<nimble-some-control style="--ni-nimble-control-height: 50px"></nimble-some-control>

Some options to consider:

1) We state firmly that nimble controls have a fixed height that is not intended to be changed by any client. In that case we should treat it like padding and make it more difficult to override control height: https://github.com/ni/nimble/issues/598. We should also consider removing the token / marking the token internal (--ni-nimble-internal-control-height) 2) We allow the controls to have user-defined heights. The --ni-nimble-control-height token is only used for setting the default height of controls and is not used in CSS layout calculations. CSS is modified to avoid requiring calc() statements using --ni-nimble-control-height. 3) [not recommended] We support users configuring the height on controls using the --ni-nimble-control-height design token. I do not think we should encourage users to use a CSS property for sizing controls that is proprietary to the nimble design system. We should discourage / forbid users setting --ni-nimble-control-height directly.

Predesigned Sizing

Many new designs for nimble components have additional heights, like the new nimble-switch Figma Design and nimble-button Figma Design. The button design now includes heights of 24px for text buttons and 18px / 16px for icon buttons, but currently the only height implemented for buttons is 32px. These other heights for buttons have use cases that are mentioned in the "Sizes" section of the Nimble Button Usage Guidance. For example:

From "Sizes" of the Nimble Button Usage Guidance

32px buttons are the default sized control.  Use this control size first.

24px buttons are the secondary control size and should be used if there is limited space.  This size should not be used in combination with 32px controls.

18px buttons...

Because of these use cases, these heights for the button (and any other component with multiple heights specified in a Figma Spec) should be implemented as an enhancement.

Current direction

Based on current discussions the expected direction is (2). The control height custom property should only be used to configure the default height of a control. It should not be used in calculations or as a variable meant to be updated internally in a control. For programmatic observation of a control height ResizeObserver should be used (with due consideration for performance)

As for the predesigned sizing, this should be implemented regardless of the decision of arbitrary sizing, as these sizes / heights have specific pre-defined use cases.

jattasNI commented 2 years ago

@NIbokeefe says he's received requests for 24px height controls already (mostly for desktop use cases), so configurable height is something we'll need to be able to support over the long term. For that use case we would likely want to provide a way for apps to set the height of all controls at once, not have to set the height on each individual control. We still need more use cases before deciding whether to support arbitrary heights, but this tech debt is worth addressing.

When we're ready to tackle the work of having each control support new sizes, he would give us some general guidance about how to distribute the space between borders, content, fonts, etc and we could hopefully implement that with token changes. Eventually he will add examples for each control to XD, but that's a lot of work so we shouldn't wait for that.

rajsite commented 1 year ago

We should include some regression tests to make sure controls respond well to different heights

rajsite commented 2 months ago

This issue should apply to icon-size as well as control-height and we should make sure to update docs and other references (i.e. some stack overflow posts) when we have a consistent direction