Show alpha warning for service instances #512
Conversation
There was a problem hiding this comment.
Personally I'd not bother with adding logic to "hide" the warning message unless we make it permanent by storing the setting in local storage or something. Which I do not think is worth doing either since this is temporary.
On its current shape, in my opinion, the fact that we allow people to temporarily hide it is more annoying than useful.
I'd just personally add the warning by using a generic warning flash message and the "alpha" suffix in the menu, all this trying to reduce the amount of code added.
dashboard/src/actions/catalog.ts
Outdated
| @@ -49,6 +49,10 @@ export const errorCatalog = createAction( | |||
| type: "ERROR_CATALOG", | |||
| }), | |||
| ); | |||
| export const receiveDisableAlphaWarning = createAction("DISABLE_WARNING", () => ({ | |||
| showAlphaWarning: false, | |||
There was a problem hiding this comment.
Do you need to pass showAlphaWarning in the action?
| @@ -15,6 +16,7 @@ import { | |||
| } from "../ErrorAlert"; | |||
| import PageHeader from "../PageHeader"; | |||
| import SearchFilter from "../SearchFilter"; | |||
| import AlphaWarning from "./AlphaWarning"; | |||
There was a problem hiding this comment.
Instead of creating another warning message I'd try to reuse the generic ones that we already have.
|
I am fine removing the logic to hide the message |
| </MessageAlert>, | ||
| ); | ||
| expect(wrapper.text()).toContain("test"); | ||
| expect(wrapper.find(".message__content").text()).toContain("test"); |
There was a problem hiding this comment.
should we add some snapshots here so we can tell if things have structurally changed? I think they can be useful as indicators of changes, and also can be easier for writing simple tests like this where you're just checking content.
There was a problem hiding this comment.
I'd like to raise a discussion for that. I don't find snapshots very helpful because it doesn't allow you to express intentionality. How would you say that the important part of the snapshot is that it renders a element with the class message_content with the text test?
There was a problem hiding this comment.
I agree with that, I think snapshots are useful in addition to more specific assertions as they can catch unexpected structural changes.
| @@ -41,7 +41,7 @@ class Header extends React.Component<IHeaderProps, IHeaderState> { | |||
| to: "/charts", | |||
| }, | |||
| { | |||
| children: "Service Instances", | |||
| children: "Service Instances (Alpha)", | |||
|
|
||
| it("renders the warning for alpha feature", () => { | ||
| const wrapper = shallow( | ||
| <InstanceListView |
There was a problem hiding this comment.
you may be able to simplify this by casting a smaller object to the InstanceListViewProps (e.g. https://github.com/kubeapps/kubeapps/blob/ea625419021199bc82a1365d75d5ab60c0660fa7/dashboard/src/components/SchemaForm/FieldTemplate.test.tsx#L8), we may need to export InstanceListViewProps.
There was a problem hiding this comment.
I could omit some of the parameters but not all of them. Thanks!
| @@ -20,7 +21,7 @@ import { InstanceCardList } from "./InstanceCardList"; | |||
| export interface InstanceListViewProps { | |||
| brokers: IServiceBroker[]; | |||
| classes: IClusterServiceClass[]; | |||
| error: Error; | |||
| error: Error | undefined; | |||
There was a problem hiding this comment.
can this be error?: Error? Sometimes Typescript complains about this though.
There was a problem hiding this comment.
I couldn't but I can omit the error parameter so I don't need that change.
| @@ -4,17 +4,20 @@ import { Info } from "react-feather"; | |||
| import ErrorPageHeader from "./ErrorAlertHeader"; | |||
|
|
|||
| interface IMessageAlertPageProps { | |||
| header: string; | |||
| header?: string; | |||
| type?: string; | |||
There was a problem hiding this comment.
nitpick: level might be a better word for this
|
I am going to merge this PR to unblock a possible release but let me know if we should review the snapshot policy |
Fixes #475
[EDIT]