iTwin / iTwinUI

A design system for building beautiful and well-working web interfaces.
https://itwin.github.io/iTwinUI/
MIT License
105 stars 38 forks source link

Checkbox: Checkmark is not visible if scss/classes.scss or css/all is imported scoped under a selector #521

Closed ajmiam closed 2 years ago

ajmiam commented 2 years ago

Environment

Code Sandbox

Link to a minimal repro (fork this code sandbox):

Steps to reproduce

I am developing a large component that is used in multiple applications within Bentley. As suggested by the internal Bentley UX site, in order to use the standard iTwinUI class names and get the usual styling, I have enclosed my app in a div with a certain class name (in this example, "itwin-app-boundary") and then imported the iTwinUI style rules as follows within my .scss file:

.itwin-app-boundary {
  @import "~@itwin/itwinui-css/css/all";
}

Within this app boundary element, I created a checkbox and associated label, giving both of them the appropriate iui classes.

Actual behavior

Although the checkbox itself and the label are styled according to itwinui standards, the checkmark never appears, even when the box should be checked. (Examining through the console, the state of the box is changing between checked and unchecked when clicked, like usual, but its state is not visible in the box as a checkmark.)

Expected behavior

The checkmark should appear and disappear appropriately when the box is clicked.

Additional information

As shown in the comments of styles.scss, I tried adding some imports to some of the itwinui styles in the global scope of my stylesheet, thinking it would import variables that might be missing. However, it didn't help. The checkbox does work if I import css/all into the global scope of my stylesheet, but I don't want to do that since it could affect the app that consumes my component.

Also, importing scss/classes.scss does not work as expected, whether it is inside the .itwin-app-boundary rule or not. Inside the .itwin-app-boundary rule, it behaves the same as css/all inside that rule (affecting the box and label, but not causing the checkmark to show up). Outside the app boundary rule, it turns the checkbox blue but doesn't cause a checkmark to appear in it.

mayank99 commented 2 years ago

Hi @ajmiam 👋 thanks for the issue.

I'm curious about three things:

I should mention that the Bentley UX documentation is outdated and we don't really recommend following that "getting started" guide anymore.

ajmiam commented 2 years ago

Hi, Mayank,

 Thank you for taking a look at the issue.

 To clarify the situation:

--Andrew Menzies

From: Mayank @.> Sent: Thursday, March 10, 2022 1:45 PM To: iTwin/iTwinUI @.> Cc: Andrew Menzies @.>; Mention @.> Subject: Re: [iTwin/iTwinUI] Checkbox: Checkmark is not visible if scss/classes.scss or css/all is imported scoped under a selector (Issue #521)

WARNING: This email originated from outside of the organization. DO NOT click links, open attachments, or respond unless you recognize the sender and know the content is safe.


Hi @ajmiamhttps://urldefense.com/v3/__https:/github.com/ajmiam__;!!F1Q1IbZmrAg!VGOVa5FJjQgs3iCGy4SKdPQr4XKbp_DnQZzoY3tYDWylIiBLkdmLdALAN-Msr5TINHpP$ 👋 thanks for the issue.

I'm curious about three things:

I should mention that the Bentley UX documentation is outdated and we don't really recommend following that "getting started" guide anymore.

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/iTwin/iTwinUI/issues/521*issuecomment-1064378785__;Iw!!F1Q1IbZmrAg!VGOVa5FJjQgs3iCGy4SKdPQr4XKbp_DnQZzoY3tYDWylIiBLkdmLdALAN-Msr_ZYgLIv$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AGCIZ6VN2GW7726RQ5TBBKDU7I7LRANCNFSM5QNNYXLQ__;!!F1Q1IbZmrAg!VGOVa5FJjQgs3iCGy4SKdPQr4XKbp_DnQZzoY3tYDWylIiBLkdmLdALAN-Msr9OpRoXj$. Triage notifications on the go with GitHub Mobile for iOShttps://urldefense.com/v3/__https:/apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!F1Q1IbZmrAg!VGOVa5FJjQgs3iCGy4SKdPQr4XKbp_DnQZzoY3tYDWylIiBLkdmLdALAN-Msr9xqxm5a$ or Androidhttps://urldefense.com/v3/__https:/play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!F1Q1IbZmrAg!VGOVa5FJjQgs3iCGy4SKdPQr4XKbp_DnQZzoY3tYDWylIiBLkdmLdALAN-Msr3BnIlxg$. You are receiving this because you were mentioned.Message ID: @.***>


This email, including any attachments, may contain confidential and/or proprietary information intended only for the use of the recipient. If you are not the intended recipient, any distribution, copying, or use of this email or its attachments is prohibited. If you received this email in error, please reply to the sender immediately and delete this message and any copies.

Bentley Systems has taken all reasonable steps to ensure that this communication is free from viruses, data corruption, and unauthorized alteration. Bentley Systems does not accept liability for any damages that may be incurred as a result of this or any communication by email

[https://cdn2.webdamdb.com/310th_sm_UnR3pO7k0ir0.jpg?1616176329]

mayank99 commented 2 years ago

Hi @ajmiam

The UX site writeup is definitely outdated in this case, however, we don't really have a recommendation for your situation as we haven't encountered it before.

The problem with depending on itwinui-css is that it's still pre-1.0 and we make breaking changes in minor version bumps. For this reason, we recommend apps to rely on the stable component API from itwinui-react where breaking changes are consumed safely. If you lock down your package to a specific version of itwinui-css, it can cause issues in apps that depend on itwinui-react. See https://github.com/iTwin/iTwinUI-react/wiki/Version-conflicts

The issue described above can happen even if you are scoping all.css inside a class. This is the reason I was suggesting including the mixins inside your own selectors. But I recognize that this is a tedious process and might not be comfortable from your perspective.

Out of curiosity, can you list the components that your package will be using? We were under the assumption that no one was using our CSS directly, so this is interesting for us to know about as we slowly start to stabilize our CSS API.

mayank99 commented 2 years ago

I thought about it some more. There will be breaking changes over the next few months (until 1.0 release). There is simply no way around it. So if you're choosing to build your UI using our CSS directly, then you will need to keep up with the breaking changes one way or another. Ideally you would rely on the latest version of itwinui-css and just check our releases every ~2 weeks, as well as have visual tests for safety. It's not too hard to consume the breaking changes as usually it's just updating some class names or moving some elements around. You will find the exact changes you need to make in our html pages. For example, if you look at 0.48.0, it was a big release, and you can find every single change at https://github.com/iTwin/iTwinUI/compare/v0.47.0...v0.48.0.

So the question remains whether you want to do the upfront work of dealing with the mixins or use the selectors or have the itwin app boundary like you were initially trying. Since you need to keep up with the new releases anyway, I would suggest just using the selectors directly.

Fwiw, I was able to make your sandbox work by importing the theme variables (which normally get set on the root element). image

But as I said in my previous comment, this scoping approach does not automatically guard the user's app.

mayank99 commented 2 years ago

Closing this, as there is no "bug".