-
Notifications
You must be signed in to change notification settings - Fork 9
feat(components): move the snippet and copy to code components from gonfalon to LP #1795
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
feat(components): move the snippet and copy to code components from gonfalon to LP #1795
Conversation
🦋 Changeset detectedLatest commit: 93ac482 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 |
| @@ -0,0 +1,132 @@ | |||
| @import 'prism-themes/themes/prism-ghcolors.css'; | |||
|
|
|||
| .snippet { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed this from Snippet to snippet
| width: 100%; | ||
| } | ||
|
|
||
| .copyable [class*='_CopyToClipboard'] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed this from Snippet--copyable to copyable
| border: none; | ||
| } | ||
|
|
||
| .snippet:not(.useDefaultHighlighting) .line-highlight { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed Snippet--useDefaultHighlighting to useDefaultHighlighting
| }: SnippetProps) { | ||
| const codeEl = useRef<HTMLElement>(null); | ||
|
|
||
| // biome-ignore lint/correctness/useExhaustiveDependencies: children and lang are intentionally included to re-highlight when they change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the biome-ignore to maintain the existing dependency list so we don't cause any unintended regressions with this migration
packages/components/src/utils.tsx
Outdated
| return true; | ||
| } catch (error) { | ||
| announce('Failed to copy', 'polite', 300); | ||
| console.error(`Unable to copy: ${error}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what to do about this, we were previously sending up a trackError and throwing this error message. Is there a pattern we want to establish where we can throw a CustomEvent that the consumer listens for if there's a failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous trackError solution wasn't very useful anyway so I think we can safely just announce and remove the console.error and not do anything else.
commit: |
|
Size Change: -9 B (0%) Total Size: 533 kB
ℹ️ View Unchanged
|
| "@launchpad-ui/icons": "workspace:~", | ||
| "@launchpad-ui/tokens": "workspace:~", | ||
| "class-variance-authority": "0.7.0" | ||
| "@react-aria/live-announcer": "3.4.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in devDep? I see all the other @react-aria deps are devDeps. I'm not sure why, but best to stay consistent. Better still find out why they are devDep and not dep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure on that one. The reason being that we had originally had it as a dependency in the dependencies list and I didn't want to stray from that. What'd you think?
https://github.com/launchdarkly/gonfalon/blob/main/packages/forms-experimental/package.json#L6-L28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments, but otherwise lgtm.
packages/components/package.json
Outdated
| "@react-aria/live-announcer": "3.4.4", | ||
| "class-variance-authority": "0.7.0", | ||
| "prism-themes": "1.9.0", | ||
| "prismjs": "1.30.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how big prismjs is? Do we want to package this into @launchpad-ui/components like this? (I'm not sure yet, but curious if this is something you all thought about already.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yusinto and I talked about it. I'm not sure if we want is a peer dependency, or as a direct dependency. The package size for a minified bundle is at 18.7K (minified and gzipped at 6.9K)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel like a peer dependency would work well here? Curious what @vezaynk thinks too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My general idea around making everything react-aria into peer dependencies is because having differing versions actually broke things.
I don't think the situation is the same with prismjs. The other argument for peers is that we want to force dedupping to save on size. I think that's valid too.
More generally, I think erring on the side of peer dependencies might be cool because it will avoid needing to do dependency version bumps here.
tldr: No strong feelings from me, but I have been enjoying peer dependencies lately (especially post-PNPM).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong feelings. Peer dep is fine with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll set it as a peer dependency
|
Is this related to us trying to share onboarding code between different code bases? |
It is! We're looking to share the Snippet in our docs site too |
c2c24d8 to
a714c81
Compare

Summary
https://launchdarkly.atlassian.net/browse/REL-10213
Screenshots (if appropriate):
Testing approaches
Related Jira issue: REL-10213: Move Snippet and CopyToClipboard to launchpad-ui