-
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 empty and loading states to Plots webview with tests #1000
Conversation
In case you didn't see it: #994 (comment) |
|
||
const signalInitialized = () => | ||
vsCodeApi.postMessage({ type: MessageFromWebviewType.initialized }) | ||
|
||
const App = () => { | ||
export const App = () => { |
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 App
has 29 lines of code (exceeds 25 allowed). Consider refactoring.
} | ||
window.addEventListener('message', messageListener) | ||
signalInitialized() | ||
return () => window.removeEventListener('message', messageListener) |
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.
Cleaning up a memory leak that was exposed by tests re-creating the App
in the same context.
function acquireVsCodeApi(): InternalVsCodeApi | ||
} | ||
|
||
export const vsCodeApi = acquireVsCodeApi() |
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.
Couldn't find a way to mock this global directly, so wrapping it is.
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] Did you see that we mock this for experiments? See webview/src/experiments/components/App.test.tsx
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.
We also mock a wrapper there, webview/src/experiments/model/vsCodeApi
.
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.
why not reuse the code?
That's a good point, I hadn't considered the other types of plots in this Webview. With that in mind, I still think this is a good half-step to implement, then after the split sections are introduced we can change these states to instead be selectively rendered sections. |
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.
A couple of points on the content. Good iteration.
<> | ||
<p>There are no experiments to make plots from.</p> | ||
<p> | ||
<a href="command:dvc.runExperiment">Run an Experiment</a> to add some! |
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] This is not necessarily true (no checkpoints).
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.
also need to pass dvcRoot
(multi-repo workspace)
@@ -121,6 +124,13 @@ const Plots = ({ plotsData }: { plotsData?: PlotsData }) => { | |||
/> | |||
))} | |||
</> | |||
) : ( | |||
<> | |||
<p>There are no experiments to make plots from.</p> |
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.
Sure! I did the same thing we do for the table with no data, but the ugliness is more apparent here since this state can happen much more often.
Code Climate has analyzed commit 78d7d7b and detected 1 issue 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 96.3% (0.0% change). View more on Code Climate. |
Take a look at #999, I've incorporated these changes there and with the static data. We can still roll with this but that means I've have to fix conflicts all the way through the chain of 5
Closing in favor of #999, which includes most of this PR's changes |
Fixes #991
As an alternative to #994, this PR keeps plots data the same and adds different static content for two situations:
When the webview is first initialized but no data has arrived (
plotsData
=undefined
)When the webview is initialized but given empty data (
plotsData
=[]
)plots-empty-state-demo.mp4
There are also tests for these states
As this comment on #994 describes, given there will be multiple types of plots this will likely not even be close to the final appearance for these states. Nonetheless, I think the addition of tests and various code improvements make this a step in the right direction that we can build off of.