-
Notifications
You must be signed in to change notification settings - Fork 15
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
Remove connection settings when using wanda #1094
Remove connection settings when using wanda #1094
Conversation
73fe7ac
to
528aa33
Compare
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'm a bit lost with the changes.
If the intention is to use the ChecksSelectionNew
directly when we go to the settings i wouldn't touch the ClusterSettings
code, I would directly render the selection page in the trento.jsx
file:
<Route
path="clusters_new/:clusterID/settings"
element={<CheckSelectionNew />}
/>
We can even think of changing the path from settings
to something else.
The setting component looks like it will be removed soon, so I wouldn't do changes there if possible
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 totally agree with the approach @arbulu89 is suggesting, do see any potential issue in that?
421d57e
to
9b47f03
Compare
Alright, I followed the suggestion with the only difference that instead of binding directly How does this work? |
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.
Much better now! I'm sure the strangling of the old part will be much easier 😅
Some more stuff to take care off then we are ok I think 👍
assets/js/components/ClusterDetails/ChecksSelectionNew.test.jsx
Outdated
Show resolved
Hide resolved
assets/js/components/ClusterDetails/ChecksSelectionNew.test.jsx
Outdated
Show resolved
Hide resolved
assets/js/components/ClusterDetails/ChecksSelectionNew.test.jsx
Outdated
Show resolved
Hide resolved
assets/js/components/ClusterDetails/ChecksSelectionNew.test.jsx
Outdated
Show resolved
Hide resolved
assets/js/components/ClusterDetails/ChecksSelectionNew.test.jsx
Outdated
Show resolved
Hide resolved
d9fa63d
to
bbf250e
Compare
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 job! Thanks @nelsonkopliku
assets/js/components/ClusterDetails/ClusterSettingsNew.test.jsx
Outdated
Show resolved
Hide resolved
bbf250e
to
f1ec611
Compare
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.
One more change then we are ok to merge 👍
assets/js/lib/test-utils/index.jsx
Outdated
export const withExtendedDefaultState = (component, additionalState = {}) => | ||
withState(component, { | ||
...defaultInitialState, | ||
...additionalState, | ||
}); | ||
|
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'd rather export defaultInitialState
and then compose it using withState
inside the test
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.
Initially I started like that and then it looked natural following the same with...
pattern, to keep the same level of encapsulation for defaultInitialState
, composability and to reduce a bit the boiler of merging the default state with a piece of extra state when needed.
However since you prefer that, I changed as per the suggestion so we can proceed.
f1ec611
to
4223d5a
Compare
Description
This keeps only the checks selection when in Wanda context.
As a proposal, page title has also been changed from
Cluster Settings for <cluster-name>
toChecks Selection for <cluster-name>
.How was this tested?
Added a test case in
ChecksSelectionNew.test.jsx