-
Notifications
You must be signed in to change notification settings - Fork 366
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
change: [M3-8175] - Refactor KubeConfigDrawer to use CodeBlock and add Codeblock story #11019
change: [M3-8175] - Refactor KubeConfigDrawer to use CodeBlock and add Codeblock story #11019
Conversation
commandType: string; | ||
language: 'bash'; | ||
handleCopyIconClick?: () => void; |
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 added handleCopyIconClick
to optionally override the component's internal handling of the copy icon which assumes the component is being used in Linode Create
heads up - there's a few conflicts that need to be resolved |
4668a1f
to
9618942
Compare
@coliu-akamai thanks for the heads-up! Just rebased with develop to resolve conflicts If you run into issues pulling, you might need to delete your pulled down branch and |
Coverage Report: ❌ |
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.
PR looks good! 👍
9618942
to
550de7f
Compare
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.
✅ confirmed storybook works
✅ confirmed codeblock in KubeConfig
✅ reviewed codeblock in the LinodeCreate flow, looked/copied as expected
thanks Hana for the additional cleanup! 🎉
Wondering if we'd want to add comments for fields like command
, handleCopyIconClick
, ldTrackingKey
?
Is it worth adding couple of tests for CodeBlock
as well? I know it's a relatively simple component in comparison with others tho
@coliu-akamai It's a pretty simple component so I don't think it needs any tests itself, but the other components using CodeBlock such as in the Looks like the console error is an existing issue so I created M3-8713 to look into that |
Cloud Manager E2E Run #6625
Run Properties:
|
Project |
Cloud Manager E2E
|
Branch Review |
develop
|
Run status |
Passed #6625
|
Run duration | 24m 54s |
Commit |
34d5d1f83a: change: [M3-8175] - Refactor KubeConfigDrawer to use CodeBlock and add Codeblock...
|
Committer | Hana Xu |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
2
|
Pending |
2
|
Skipped |
0
|
Passing |
414
|
View all changes introduced in this branch ↗︎ |
Description 📝
Refactor KubeConfigDrawer component to use the CodeBlock component for UI consistency. I also moved CodeBlock to the components folder since it is pretty generic and added a story for it
Changes 🔄
List any change relevant to the reviewer.
KubeConfigDrawer
to useCodeBlock
component and minor cleanupCodeBlock
fromfeatures
tocomponents
folder and decoupled it a bit from Linode CreateCodeBlock
storyPreview 📷
(info has been obscured)
How to test 🧪
Prerequisites
Verification steps
(How to verify changes)
Create a cluster pointing to dev API
Once the cluster is provisioned, go to the cluster's details page
Click on View under Kubeconfig
Confirm new UI and no regressions
Delete the cluster afterward
Run storybook locally and confirm CodeBlock story
As an Author I have considered 🤔
Check all that apply