Loader components on applications and charts #644
Conversation
migmartri
changed the title
| beforeEach(() => { | ||
| props = { ...defaultProps, apps: { isFetching: true } }; | ||
| }); | ||
| context("while fetching apps", () => { |
There was a problem hiding this comment.
what's the difference between context and describe?
There was a problem hiding this comment.
It is just syntax sugar. I like using describe to mention the test subject and context to set scenarios.
It might now be worth adding the library that's why I did not load it globally, let me know if you'd prefer not to have it.
There was a problem hiding this comment.
okay, was just wondering if there was actually some difference, thanks!
There was a problem hiding this comment.
that module has been deprecated: https://www.npmjs.com/package/jest-context. If you want to add it apparently you should use "jest-plugins". In any case I don't think we should add it, as happened with @prydonius or myself, other people may be confused with the context usage. Also, in my opinion, we should try to use avoid using third-party modules if possible (to avoid possible vulnerabilities or license issues).
There was a problem hiding this comment.
Good catch @andresmgot.
Also, in my opinion, we should try to use avoid using third-party modules if possible (to avoid possible vulnerabilities or license issues).
This is tricky, because this single module is nothing compared to all the dependencies from our main components.
I do not see any license issue in this case since it is MIT.
In any case I don't think we should add it, as happened with @prydonius or myself, other people may be confused with the context usage.
What do you find confusing? Having two methods? This concept comes from rspec in which describe and context are used for different things. Context used to setup a context or scenario and describe to express the test subject.
I can remove it and use nested describes but honestly that looks odd and adding context helps with tests readability (at least for me). If you are strongly opinionated against it I can remove it.
There was a problem hiding this comment.
It's a minor thing, I know that in this case it's just a minor package with a single line. I am not against it so go ahead if you want to use it. I guess I am more used to see nested describes so I don't find it weird.
|
|
||
| export default { | ||
| loadingSpecs, | ||
| export default (sharedExampleName: string, args: any) => { |
|
|
||
| // Although LoadingWrapper checks that the app is loaded before loading this wrapper | ||
| // it seems that react renders it even before causing it to crash because app is null | ||
| // that's why we need to have an app && guard clause |
There was a problem hiding this comment.
(regarding the comment) isn't it better then to use in render?:
return isLoading ? <LoadingWrapper ... /> : {this.appInfo()}
I would find that easier to understand, what's the benefit of setting appInfo as children?
There was a problem hiding this comment.
I would find that easier to understand, what's the benefit of setting appInfo as children?
That's a good point. I can change that. There is not an actual benefit apart of you do not need to add logic to show one or the other. That said, the fact that I need to do app actually defuses the purpose.
| beforeEach(() => { | ||
| props = { ...defaultProps, apps: { isFetching: true } }; | ||
| }); | ||
| context("while fetching apps", () => { |
There was a problem hiding this comment.
that module has been deprecated: https://www.npmjs.com/package/jest-context. If you want to add it apparently you should use "jest-plugins". In any case I don't think we should add it, as happened with @prydonius or myself, other people may be confused with the context usage. Also, in my opinion, we should try to use avoid using third-party modules if possible (to avoid possible vulnerabilities or license issues).
migmartri
changed the title
Closes #165
Adds loader components to the app view, deployment and upgrade components, as well as enhances the shared test scenarios.
NOTE: I have decided to go with without children because otherwise it will require quite a refactor in some cases like in the upgrade component.