-
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
Add Checks customization modal #3277
base: main
Are you sure you want to change the base?
Conversation
The preview environment for this pull request is ready at 3277.prenv.trento.suse.com. |
9abdc66
to
ce96bfa
Compare
29407b9
to
fa3976e
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.
Hey @EMaksy great work, thanks!
Left a few comments that would allow a simpler wire up with #3274 and #3278
Here's also a temporary branch wiring up all the things checks_customization_modal...tmp-merge
fa3976e
to
439056b
Compare
439056b
to
fbf76c1
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.
Very good @EMaksy, just a couple of comments about factories, and that would be enough to merge this PR.
I believe that this is a good starting point and besides what you mentioned let's keep in mind we'll need to make sure to provide the correct ux based on th different types of values:
- string -> input text
- number -> input number
- boolean -> radio button
onChange={(inputEvent) => | ||
handleCustomValueInput(value?.name, inputEvent.target.value) | ||
} | ||
placeholder={value?.current_value} |
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.
thought: we might not need a placeholder at all. There will always be either the current value or the custom value.
We can safely remove this imho.
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 played a bit around with it. I prefer to keep the placeholder, because when the user clicks on the field and starts to enter values, he starts from scratch otherwise he starts on the previous value.
We can revisit this behavior on the follow up for the different input types.
What do you think ?
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 guess what you want to achieve is setting an initial value for the input
initialValue={value?.custom_value || value?.current_value}
See a709946#diff-7d61854c35d8df403e17910b9865976e0ca7a475035de0ad1fa98fa4fb28ffb3R107
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.
Yeah it is basically which behaviour we prefer:
initialValue={value?.custom_value || value?.current_value}
Screencast.From.2025-02-07.15-37-00.mp4
vs
placeholder={value?.current_value}
Screencast.From.2025-02-07.15-37-44.mp4
You prefer initialValue={value?.custom_value || value?.current_value}
?
No strong opinion here, but i would need to adjust slightly 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.
The fact is that a placeholder is semantically different from the value of an input.
Additionally, something like placeholder={value?.current_value}
would always show current_value
and never the custom_value
, when present.
Description
This PR will add a checks customization modal for host and cluster checks selection view.
In a future PR the placeholder "Value from Wanda" will be replaced with the custom check values from wanda
What is msising?
Demo
Single check value unchecked
Single check value checked
Multiple customization values
Partial customizable values
Many values with a very long name
How was this tested ?
Added tests for the Modal
Added Storybook stories