-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add storage types details to remote setup page #3996
Conversation
export const StorageSlider: React.FC = () => ( | ||
<div className={styles.storageSlider}> | ||
<VSCodePanels> | ||
{storageTypes.map(storageType => ( |
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.
Similar blocks of code found in 2 locations. Consider refactoring.
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.
This is clearly a rip-off of the CodeSlider
but I feel like it is different enough to have a separate component. WDYT?
@@ -18,7 +18,6 @@ import { TooltipIconType } from '../../shared/components/sectionContainer/InfoTo | |||
import { setupReducers } from '../store' | |||
|
|||
jest.mock('../../shared/api') | |||
jest.mock('../../shared/components/codeSlider/CodeSlider') |
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.
[Q] @sroy3 is there a reason that this is under shared components? Shouldn't it be under the setup webview?
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 can't remember, but I think it might be because there was no data at all that would make it purely a setup component, and it could be reused anywhere if necessary. Components were still pretty big back then, so I saw the shared
components as being the dumb components would go. A little like a component library. But it's not really the direction we are moving in at the moment. It's fine to move it.
|
||
export const VSCodePanels = MockComponentWithChildren | ||
export const VSCodePanelTab = MockComponentWithChildren | ||
export const VSCodePanelView = MockComponentWithChildren |
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.
[F] Once we mock these components we no longer need jest.mock('../../shared/components/codeSlider/CodeSlider')
in the setup App tests.
@@ -0,0 +1,31 @@ | |||
import React from 'react' |
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.
[F] I have split these components right out to a very granular level.
[Q] Have I gone too far?
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 would have gone further as AmazonS3
, AzureBlobStorage
and GoogleCloudStorage
share a wrapper <div className={styles.storageDetails}>
and a <CloudVersioning>
block, so I don't think you went too far. It's fine as is, though.
Code Climate has analyzed commit 4e0cded and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.2% (0.1% change). View more on Code Climate. |
Part of #3867
This PR adds a
<VSCodePanels/>
component with a tab for each of the three main storage types and a fourth tab for the remaining types.The panels will be shown no matter what state the remotes section is in. The information in these pages is lifted with some slight adjustments from the following docs pages:
I have included details on how to auth, how to enable cloud versioning and a link to the docs for custom authentication.
Demo
Screen.Recording.2023-05-29.at.1.39.48.pm.mov