Add get started component to plots webview #1718
Conversation
| @@ -0,0 +1,14 @@ | |||
| /* eslint-disable unicorn/filename-case */ | |||
There was a problem hiding this comment.
[F] I spent around 2 hours trying to get jest to load @microsoft/fast-react-wrapper which is an esm dependency of @vscode/webview-ui-toolkit/react. In the end I had to give up and mock the damn thing.
| hasSelectedRevisions?: boolean | ||
| } | ||
|
|
||
| export const GetStarted = ({ |
There was a problem hiding this comment.
Please LMK if there is a better pattern to get these buttons and wrappers etc into the React App.
There was a problem hiding this comment.
I think I should start removing React.FC typings everywhere. I never questioned it and thought it was how things had to be typed, but your pattern of typing the component
export const comp = (props: CompProps) => {
...
}
vs
export const comp: React.FC<CompProps> = (props) => {
...
}
is actually a better way to type these. (facebook/create-react-app#8177)
Thanks for making me look this up.
There was a problem hiding this comment.
I was also under the impression React.FC was best practice. Implicit children is removed from the FC type in React 18, so doing so now will help when we get around to that.
There was a problem hiding this comment.
Yep this was inspired by me trying (and failing) to upgrade to React 18
mattseddon
force-pushed
the
improve-empty-state
branch
from
d1592d8 to
ead0a70
Compare
| return ( | ||
| <EmptyState> | ||
| <div> | ||
| <p>No Plots to Display</p> |
There was a problem hiding this comment.
[Q] What could we replace this with to make it look better?
|
Code Climate has analyzed commit ead0a70 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 96.7% (0.1% change). View more on Code Climate. |
|
@mattseddon looks really good, thanks. let's merge it. Next step / question - what if there are no plots setup in the repo at all? can we show some preview and link to the docs? |
| hasSelectedRevisions?: boolean | ||
| } | ||
|
|
||
| export const GetStarted = ({ |
There was a problem hiding this comment.
I think I should start removing React.FC typings everywhere. I never questioned it and thought it was how things had to be typed, but your pattern of typing the component
export const comp = (props: CompProps) => {
...
}
vs
export const comp: React.FC<CompProps> = (props) => {
...
}
is actually a better way to type these. (facebook/create-react-app#8177)
Thanks for making me look this up.
| onClick: () => void | ||
| text: string | ||
| isNested?: boolean | ||
| children?: React.ReactNode |
There was a problem hiding this comment.
When using React.FC< ButtonProps> children will be implicit and there is no need of adding it in the prop type. But like I mentioned in my previous comment, we probably should remove React.FC types everywhere instead and add children where needed. That way we can specify if it's optional or not and if it's expecting one or more nodes like you did here.
|
I'll follow up with the comments provided |
Closes #1672
This PR improve the empty state of our plots webview by adding
@vscode/webview-ui-toolkit/reactbuttons to the page whenever the user needs to select either experiments or plots to make something appear.Demo
Screen.Recording.2022-05-16.at.3.09.37.pm.mov