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.

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 error state to nimble-checkbox #2478

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add error state to nimble-checkbox #2478

wants to merge 15 commits into from

Conversation

mollykreis
Copy link
Contributor

@mollykreis mollykreis commented Nov 19, 2024

Pull Request

🀨 Rationale

This is part of #2018 and also resolves #2091

πŸ‘©β€πŸ’» Implementation

The nimble-checkbox now implements ErrorPattern to add error-text and error-visible to its API.

The bulk of the implementation is updating the template and styles of the checkbox. These changes include:

  • Adding error icon/text to the template with appropriate styles
  • Fix the wrapping behavior to keep the checkbox aligned with the top line of text rather than centered within the control
  • Update the checkbox to have a default min-height of controlHeight (see nimble-checkbox has incorrect heightΒ #2091)

πŸ§ͺ Testing

  • Existing unit tests pass
  • Manually tested in storybook
  • Extended matrix tests to cover more cases -- error states and wrapping text

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

}

:host(.checked:not(.indeterminate)) slot[name='checked-indicator'] {
display: contents;
}

slot[name='checked-indicator'] svg {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't a required change as part of this PR, but I noticed that slot[name='checked-indicator'] svg was listed twice in the styles, so I combined them.

@mollykreis
Copy link
Contributor Author

@msmithNI, will you buddy this PR for me?

@mollykreis mollykreis marked this pull request as ready for review November 20, 2024 20:08
@@ -0,0 +1,7 @@
{
"type": "minor",
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct that the styling changes will impact clients by adding additional space around their checkboxes? For example, in the example app it looks like the checkbox has some extra space on all sides so no longer left aligns with the label above it.
https://60e89457a987cf003efc0a5b-ndvckxqkhv.chromatic.com/example-client-app/#/customapp
https://nimble.ni.dev/storybook/example-client-app/#/customapp

Is that an intentional design change? If so then I think we should:

  1. mention it in the change file comment
  2. spot check a few SLE apps to ensure it looks ok (I think Fred has complained that a lot of slide outs already have bad checkbox layouts so maybe this is a low bar)

I don't think it merits being marked as a breaking change or doing additional notifications on Teams, but we could consider those too if it seems disruptive.

Copy link
Member

Choose a reason for hiding this comment

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

That padding inside the control bounds doesn't seem right. All our other controls are flush to the bounds of the control

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The padding is coming from the visual design specs. Perhaps we need to have a discussion with Brandon if we don't agree with the design.

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 either way I'd vote to split it out into a separate PR and not make the change here. If we do end up wanting it, I'd argue implementation-wise for the checkbox (which is a standalone control) that the padding / margin would be responsibility of the thing that is laying it out.

The padding is for a grouped, labelled set of checkboxes, not for every possible composition of a checkbox. We'd have a separate PR to capture the layout guidance in storybook docs or create something like a checkbox-group to help layout and we would also want to update radio button group at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll pull that change out of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The left padding has now been removed from the checkbox. The square control is back to being flush with the left edge of the nimble-checkbox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nimble-checkbox has incorrect height
4 participants