CSS conflicts integrating with existing applications and styles

SimonFinney commented 3 years ago

Affected package

@carbon/ibm-cloud-cognitive@latest

Description

Carbon global and utility styles

Importing the package CSS into an existing application can cause unintended side effects originating from global and utility styles from Carbon as a dependency - depending on the import order, this also takes precedence over any Carbon style or version that an application may already consume.

As an example, see an exploration finding from integrating the CSS into Cloud Pak for Security's application boilerplate:

Before	After

Reduced test case - https://codesandbox.io/s/cranky-galileo-7cqlz?file=/src/index.scss

Carbon component styles

There are many different solutions for removing Carbon global and utility styles, for example, toggling all global flags off as per https://github.com/carbon-design-system/ibm-cloud-cognitive/compare/SimonFinney:css-snapshot...SimonFinney:remove-carbon-utility , but there are still challenges with the package outputting styles from underlying Carbon components that conflict with the existing styles of an application.

An example is the CSS overriding the theme for the CodeSnippet:

Viewport	Code

Reduced test case - To come

Styles public API testing

As an aside, throughout the course of the exploration, we found the styles changing unexpectedly between 2 versions of this package causing side effects that added significant noise to debugging the issues outlined above.

For an example, see https://github.com/SimonFinney/ibm-cloud-cognitive/blob/css-snapshot/packages/cloud-cognitive/src/__tests__/__snapshots__/styles.test.js.snap#L2354 - any token before this line is output as hexadecimal, while anything after is output as a CSS custom property.

This brings up a need to have a better understanding and control over the stability of the public API of the package's styles, especially as teams from different parts of the organization begin to consume this package in varying ways - see #571 as a potential means of how this could be managed.

dcwarwick commented 3 years ago

So here's a proposed path forward based on our discussion the other day @SimonFinney @lee-chase:

The recommended way to use our SCSS entry points is as follows:
1. FIRST set any carbon feature flags and/or cloud-cognitive feature flags (if we have any).
2. THEN load cloud-cognitive (NB yes, before carbon. This ensures any settings we require are made) -- this can either be the whole cloud-cognitive package, or it could be one or more individual cloud-cognitive component SCSS. NB this same pattern would likely apply for any other packages built on top of carbon like us.
3. THEN load carbon. This can either be the whole carbon package, or it could be one or more individual carbon component SCSS.
If we are loaded in a different order, one or more of the carbon components we use may be imported without the settings we need, and thus things may not work as we intended: this is a decision that users of cloud-cognitive can make if they choose, so long as they are aware of this.

We should document this in our README for our users.
We should check each of the carbon feature flags we are setting, to find out whether we actually need them and what the consequences are of them not being set. Ideally, we would require no feature flags in order to operate as intended, with any feature flag usage being limited to, say, storybook etc. If we do need some feature flags set, we should be able to document what the effect will be of each flag NOT being set as we require.
We should produce a range of built CSS files, each also optionally minimised. These would be:
- index-without-carbon-released-only.css and index-without-carbon-released-only.min.css -- built CSS containing only our styles, with no carbon styles included, and only the currently released components and their dependencies. Whether or not this is consumed by downstream users it will be useful for testing as described below. To use this CSS, users would also need to import some or all of carbon as a peer. They would need to match versions, and possibly feature flags as noted above, otherwise unpredictable results might arise.
- index-without-carbon.css and index-without-carbon.min.css -- built CSS containing all our styles but only our styles, with no carbon styles included. To use this CSS, users would also need to import some or all of carbon as a peer. They would need to match versions, and possibly feature flags as noted above, otherwise unpredictable results might arise.
- index.css and index.min.css -- built CSS containing all our styles and such carbon styles as we require and depend on. Users can use this without any peer dependency to use cloud-cognitive components, but would also need to import some or all of carbon as a peer if they wish to use carbon components we do not make use of.
- index-full-carbon.css and index-full-carbon.min.css -- build CSS containing all our styles and all carbon styles. Users can use this without any peer dependency to use any cloud-cognitive and/or any carbon components. This could be a handy and simple choice for experimenting, proofs of concept, codesandboxes, etc.

dcwarwick commented 3 years ago

PS thoughts/suggestions on the exact names of those built CSS files also welcome!

dcwarwick commented 3 years ago

PPS -- we could then include a snapshot/test for index-without-carbon-released-only.css, to alert a developer to any changes that will affect styles for released components. The styles for non-released components can be presumed to still be volatile, but we should be careful about changes to released components, since even small CSS changes can be breaking (as we see regularly with carbon versions!). Also, any global or non-encapsulated styles can be readily spotted -(there shouldn't be any!). This would be my proposed adjustment to #571 opened by @SimonFinney.

SimonFinney commented 3 years ago

Thanks for putting this together! I feel it covers a significant portion of what we discussed.

To briefly add to this - Sass Carbon settings should as most as is reasonable, be defined by the consumer, and the library should aim to respect that. This would help prevent situations in which the settings of one entry point could potentially override the settings of another, as well as remove the need to import them in an explicit order.

For example, if a consumer chooses to toggle off CSS custom properties within their application, and this library decides to toggle that back on, it may cause unintended behavior that is difficult to debug. I agree that documentation would definitely draw awareness to this, but as discussed, the library should aim to work agnostic of Carbon features defined in feature flags where possible.

One thing we didn't have time for that we should continue to discuss here was how Carbon dependencies should be treated in this library's package.json - it currently includes Carbon as production dependencies, but any existing applications and libraries already managing Carbon will likely conflict with this unless the versions all match up. In this case, does it make sense to reconcile Carbon dependencies as peers within a range, and lock the versions that the library is tested against? For example:

"peerDependencies": {
  "@carbon/icons-react": "^10.24.0",
  "@carbon/import-once": "^10.5.0",
  "carbon-components": "^10.30.0",
  "carbon-components-react": "^7.30.0",
  "carbon-icons": "^7.0.7",
  ...
},
"devDependencies": {
  "@carbon/icons-react": "10.24.0",
  "@carbon/import-once": "10.5.0",
  "carbon-components": "10.30.0",
  "carbon-components-react": "7.30.0",
  "carbon-icons": "7.0.7",
  ...
},

dcwarwick commented 3 years ago

I agree that peer dependencies make more sense for carbon and for react. I've pulled the carbon version we use forward to 10.32, since there are changes in that version that PageHeader will depend upon, but I've made them peer dependencies, so it's up to our user what actual versions we are paired with. On the whole, minor changes in Carbon shouldn't make much difference, but on the other hand with CSS and DOM changes even minor changes have the potential to be breaking -- that's one of the challenges of web app building with multiple frameworks.

SimonFinney commented 3 years ago

I agree that peer dependencies make more sense for carbon and for react. I've pulled the carbon version we use forward to 10.32, since there are changes in that version that PageHeader will depend upon, but I've made them peer dependencies, so it's up to our user what actual versions we are paired with. On the whole, minor changes in Carbon shouldn't make much difference, but on the other hand with CSS and DOM changes even minor changes have the potential to be breaking -- that's one of the challenges of web app building with multiple frameworks.

I think this is a challenge that leaves risk for visual issues potentially arising, but I wonder if that's dependent on how the library constructs styles with components from Carbon. For example, I would expect little issue with the library positioning something like a button, but overriding its 'internal' styles could cause breakages through untested minor increases.

That's a very basic example, but are there other cases that need to be considered for that approach?

andrea-island commented 3 years ago

Did you all reach a resolution offline or should we keep this issue open?

carbon-design-system / ibm-products