-
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
Give Setup webview a single section #3441
Conversation
needsGitInitialized={needsGitInitialized} | ||
needsGitCommit={needsGitCommit} | ||
projectInitialized={projectInitialized} | ||
pythonBinPath={pythonBinPath} |
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] Should I just match the plots and experiments <App/>
s and bring redux in here now?
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 don't think it's worth it yet for the few levels that we have and how simple the components are.
> = ({ | ||
children, | ||
menuItems, | ||
menuItems = [], |
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] We don't need this right now but I thought maybe we could use it for a carousel-like action to move between the screens.
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.
<-
->
in the icon menu
} | ||
|
||
if (!hasData) { | ||
return <NoData /> |
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.
Avoid too many return
statements within this function.
} | ||
|
||
if (hasData === undefined) { | ||
return <EmptyState>Loading Project...</EmptyState> |
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.
Avoid too many return
statements within this function.
return <NoData /> | ||
} | ||
|
||
return <EmptyState>{"You're all setup"}</EmptyState> |
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.
Avoid too many return
statements within this function.
pythonBinPath: string | undefined | ||
} | ||
|
||
export const SetupExperiments: React.FC<SetupExperimentsProps> = ({ |
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.
Function SetupExperiments
has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
return <CliIncompatible checkCompatibility={checkCompatibility} /> | ||
} | ||
|
||
if (cliCompatible === 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.
Similar blocks of code found in 2 locations. Consider refactoring.
Code Climate has analyzed commit fc5aa0e and detected 5 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 95.1% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.7% (0.0% change). View more on Code Climate. |
needsGitInitialized={needsGitInitialized} | ||
needsGitCommit={needsGitCommit} | ||
projectInitialized={projectInitialized} | ||
pythonBinPath={pythonBinPath} |
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 don't think it's worth it yet for the few levels that we have and how simple the components are.
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!
Note: This looks odd now but there will be a section above for the walkthrough and one below for Studio. I will also condense the EmptyStates to EmptySections (isFullScreen=false)
Why not we hold off adding the collapsible section until there is more than one?
That's the next PR. I was going to hold off merging this until it is ready. |
This reverts commit a376e19.
Merged this because I thought it was a different branch. Will replace the PR 💢 😠! |
2/2
main
<- #3440 <- thisRelated to #3434.
This PR uses the shared component created in #3440 to give the Setup webview a single section.
Demo
Screen.Recording.2023-03-10.at.3.26.06.pm.mov
Note: This looks odd now but there will be a section above for the walkthrough and one below for Studio. I will also condense the
EmptyState
s toEmptySection
s (isFullScreen=false
)