fix(components): REL-10382 - update the way snippet CSS is applied so that children get colors all set #1805
Conversation
…snippets work and apply styles
🦋 Changeset detectedLatest commit: 6103ef6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ari-launchdarkly
changed the title
commit: |
|
Size Change: -655 B (-0.11%) Total Size: 601 kB
ℹ️ View Unchanged
|
packages/components/src/Snippet.tsx
Outdated
| <> | ||
| {withHeader && ( | ||
| <div className={styles.header}> | ||
| <div className="header"> |
There was a problem hiding this comment.
If we merge this as-is then any consumer of Launchpad will have these styles applied to any element with the classname .header, which is probably both unexpected and seems like a poor experience for a library to provide.
There was a problem hiding this comment.
One option is to better namespace these classnames if we're going to be using them directly. However, I think that should be an absolute last-resort. Can you explain more fully why CSS modules aren't working here?
There was a problem hiding this comment.
I hear ya. We can combine module style application and generic class application, since the intent of the PR is to enable the current setting of styles based on dynamic child HTML elements
packages/components/src/Snippet.tsx
Outdated
| )} | ||
| <div | ||
| className={`${styles.snippet} ${className ?? ''} ${withCopyButton ? styles.copyable : ''} ${useDefaultHighlighting ? styles.useDefaultHighlighting : ''}`} | ||
| className={`snippet ${className ?? ''} ${withCopyButton ? 'copyable' : ''} ${useDefaultHighlighting ? 'useDefaultHighlighting' : ''}`} |
There was a problem hiding this comment.
You should be using import { cx } from 'classix'; to make it readable to engineers
Also the recommended way of styling is to use css modules. If you need to update styles from a higher level, you should pass down classNames as a prop (eg: headerClassName etc)
There was a problem hiding this comment.
I hear ya. The issue is that we don't control the children classes. Maybe I'm misunderstanding the application here, but we're getting a bunch of classes that the snippet is supposed to handle. So for example, children can look something like this:
<span className="token function">myFunction<span className="token parenthesis">()</span></span>
Right now, we're missing out on applying the classes, hence the global CSS file and not the module application
There was a problem hiding this comment.
@zmdavis the reason above is the issue we're encountering. It's weird. Right now the snippet is responsible for verify-specific styles on an undocumented API w/r/t what the child shape should be. Ideally we'd be passing:
<span className={cx(styles.token, styles.function)}>myFunction ...
but we're just passing in a string.
There was a problem hiding this comment.
If you need to over-ride css from 'prismjs/plugins/line-numbers/prism-line-numbers.css';, you can use the :global operator
There was a problem hiding this comment.
So what the UI is doing is taking a txt file:
and injecting that as a child of the Snippet component.
There was a problem hiding this comment.
So right now, no styles are being set for classNames that are passed into children since we're relying on a modular syntax rather than a global css like we are in the current snippet component:
There was a problem hiding this comment.
I believe what Naomi is saying is still valid. We can combine css modules with "global" classnames as needed
There was a problem hiding this comment.
I'll give that a shot.
There was a problem hiding this comment.
However, I think this raises a deeper issue of how we're using this and whether this component belongs in a library like Launchpad, since it relies on global classnames that can easily conflict with styles from the consuming application.
There was a problem hiding this comment.
However, I think this raises a deeper issue of how we're using this and whether this component belongs in a library like Launchpad, since it relies on global classnames that can easily conflict with styles from the consuming application.
Ya I totally agree. I think given the current implementation, it's not correct for us to have this version of the Snippet exist for external consumption of the component. Having said that, there's a space for a well-defined version of this to consume markdown that we parse and pass classes onto, or have the consumer deal with presentational concerns. Having said that, this is something that we'll need moving forward so I'll rope @yusinto in to see if we want to localize the implementation onto the onboarding-components
packages/components/src/Snippet.tsx
Outdated
| <> | ||
| {withHeader && ( | ||
| <div className={styles.header}> | ||
| <div className="header"> |
There was a problem hiding this comment.
One option is to better namespace these classnames if we're going to be using them directly. However, I think that should be an absolute last-resort. Can you explain more fully why CSS modules aren't working here?
|
Closing this in favor of: |
Summary
The original module work breaks the existing UI since we can't apply a direct mapping from
stylesto the childrenScreenshots (if appropriate):
Testing approaches
Related Jira issue: REL-10382: Fix snippet highlight colors