-
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 Remotes section to setup webview #3901
Conversation
webview/src/shared/components/sectionContainer/SectionContainer.tsx
Outdated
Show resolved
Hide resolved
592bcec
to
cf29462
Compare
f3b9910
to
288b3dc
Compare
state, | ||
action: PayloadAction< | ||
| { [dvcRoot: string]: { [alias: string]: string } | undefined } | ||
| undefined |
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.
Can we use RemoteList
here?
Object.entries(remotes).map(([alias, remote], i) => ( | ||
<tr key={[alias, remote].join('-')}> | ||
{i === 0 ? <td className={styles.alias}>{dvcRoot}</td> : <td />} | ||
<td className={styles.alias}>{alias}</td> |
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 a th
?
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.
Unrelated to this pr, but we also should be using th
in the dvc env details table. I'll open a quick pr later correcting that :)
<tbody> | ||
{Object.entries(remotes).map(([alias, remote]) => ( | ||
<tr key={[alias, remote].join('-')}> | ||
<td className={styles.alias}>{alias}</td> |
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.
Same comment here about using a th
instead of a td
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.
Great work!
expect(setupDVCButton).toBeVisible() | ||
}) | ||
|
||
it('should show the list of remotes there is only one project in the workspace', () => { |
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:
it('should show the list of remotes there is only one project in the workspace', () => { | |
it('should show the list of remotes if there is only one project in the workspace', () => { |
Object.entries(remotes).map(([alias, remote], i) => ( | ||
<tr key={[alias, remote].join('-')}> | ||
{i === 0 ? <td className={styles.alias}>{dvcRoot}</td> : <td />} | ||
<td className={styles.alias}>{alias}</td> |
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.
Unrelated to this pr, but we also should be using th
in the dvc env details table. I'll open a quick pr later correcting that :)
remoteList: NonNullable<RemoteList> | ||
}> = ({ remoteList }) => ( | ||
<table data-testid="remote-details" className={styles.remoteDetails}> | ||
<tbody> |
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.
Minor, but we technically don't need this tbody
in the table
since a tbody
is only needed if table
has thead
, tfoot
or multiple tbody
s. We also have an unneeded tbody
in dvc env table... I'll change that :)
import { SetupSection } from 'dvc/src/setup/webview/contract' | ||
import { updateSectionCollapsed } from '../../state/webviewSlice' | ||
|
||
const getAllSections = (collaspsed: boolean) => ({ |
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.
Did you mean collapsed
?
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.
Great work on the shared
functions and components!
288b3dc
to
4093a8d
Compare
Code Climate has analyzed commit ace9c69 and detected 4 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 99.1% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.0% (0.0% change). View more on Code Climate. |
2/3
main
<- #3908 <- this <- #3912First basic step for #3867.
Demo
Screen.Recording.2023-05-17.at.6.15.07.pm.mov
See storybook for how workspaces with multiple projects are handled.