cockpit
cockpit copied to clipboard
cockpit-project
Add Minutely and Custom options to specific time trigger for creating timers
Added minutely and Custom option to the specific time trigger in the CreateTimer Dialog per conversation on issue #9467
Also fixed error from UserInputs.js TextInput not having proper id or aria-labels.
Nice, thank you for this PR!
A few comments/ideas that I have noticed when I tried this one out.
-
When I open the dialog and any time I switch to 'Don't repeat' the dropdown is opened. That is broken. Very likely not related to this PR, I will try current main and if it happens there as well I will send separate fix. Just wanted to mention it in case it got broken here:
-
Minutelyis all good! It needs test though. See for example here: https://github.com/cockpit-project/cockpit/blob/main/test/verify/check-system-services#L1149 -
Customis not clear to me what is expected as input. Can we have there some (?) icon with tooltip explaining what is expected as input? And also, it would need tests the same way.
When I open the dialog and any time I switch to 'Don't repeat' the dropdown is opened.
Happens on main, don't worry about this comment then.
When I open the dialog and any time I switch to 'Don't repeat' the dropdown is opened.
Happens on main, don't worry about this comment then.
Sent https://github.com/cockpit-project/cockpit/pull/17595
Hi. I noticed that issue too. kind of annoying. Anyway I have added the integration tests and a popover as suggested. not sure if its alright to have a link in the popover or not but I think it helps clarify the custom field. thoughts?
Current screenshot for reference:
@garrett can you please ack this? Thanks!
Thank you! Just two tiny nitpicks. I also triggered tests to see how they stand. Also could you please squash all the commits and rebase to current main, thanks!
The problem with the spacing is because of this:
It should be using helperText on the input, not the helperText component. They're different things, often used similarly. This is one of PatternFly's confusing naming issues, so it's easy to make this mistake.
Additionally, you can see that the link is outside the text area, so it's on its own line, instead of being inline in the same area.
For an example of the helperText attribute (instead of component), look at https://www.patternfly.org/v4/components/form#horizontal, which has the following snippet:
<FormGroup label="Full name" isRequired fieldId="horizontal-form-name" helperText="Include your middle name if you have one.">
<TextInput value={name} isRequired type="text" id="horizontal-form-name" aria-describedby="horizontal-form-name-helper" name="horizontal-form-name" onChange={handleNameChange} />
</FormGroup>
I think you might be able to construct a React.Fragment (<> … </>) with an inline link inside and pass that to the helperText attribute.
@garrett I was able to use a React.Fragment for this so that was nice. I also added the placeholder text as I too believe that is the correct way to do this. @marusak should be all Squashed and rebased! Thx guys!
Nice thank you!
Looks like this now:
I rebased once more as there was conflict with submodules. Running tests now
The test often fails on:
wait_js_cond(ph_is_present("#drop-repeat option[value='minutely']")): Uncaught (in promise) Error: condition did not become true
I think this might be some race condition, looking
Tests see this in journal: Failed to parse calendar specification '*:': Invalid argument
It works very nicely!
Two issues I found:
The button is disabled without explanation why. We might want to add * next to those required fields, see isRequired prop
You are able to submit without entering anything in the custom fields.
The button is disabled without explanation why. We might want to add * next to those required fields, see isRequired prop
We haven't done this in Cockpit yet, but really probably should.
Design guidelines on user guidance, starting with required: https://www.patternfly.org/v4/components/form/design-guidelines/#user-guidance
you know I was curious about that as I didnt see any required fields anywhere. Technically if we are trusting react and monitoring states properly we shouldnt need it but i do think its the right thing to do and should help simplify the submit check using checkValidity() Ill update accordingly.
@garrett and @marusak Little bit of a mess on the existing "specific time" validation routines. They validate on change currently but that only displays the helpertext. nothing prevents the submission so it creates bad timers. tested on current version. Dont know if you want a separate ticket for that or not? I have everything else ready but this should be taken care of i would think. please advise.
Little bit of a mess on the existing "specific time" validation routines. They validate on change currently but that only displays the helpertext. nothing prevents the submission so it creates bad timers. tested on current version. Dont know if you want a separate ticket for that or not? I have everything else ready but this should be taken care of i would think. please advise.
Do you have estimate how much work would it take? If it happens currently as well, then we can open issue for it and then fix it separately. I tried it out and the submit button is not disabled but it does nothing when clicking on submit. Also with empty description the submit button is already enabled, but clicking on it shows that the description cannot be empty.
@marusak I did fix the validation issues for the repeating entries which atleast prevents a bad timer from getting generated like the current version. As for submission with a bad value right now the submit function just returns which is why you dont see anything. however, the bad fields should be invalidated showing the helperText. since this type of form validation doesnt yet exist in cockpit im unsure on how we want to display error messages related to this type of failure. The helperText does show the invalid fields but maybe thats not enough? It also disables the submit button if the form is not validated. As for the save button disabling I have been struggling on how best to implement this. The issue you observed where you enter a name and the save button becomes enabled is because I have no default state in the validationFailed hook. if I set default states then all the fields are invalidated immediately which causes helper text to display right away which I would think is not what we want.
I guess my thought on it was even if the save button is enabled the submit function invalidates the submission and disables the button if the form is not complete. Since this does prevent bad form submission I guess my thought was lets get the custom field added to the main and fix the now known bug related to the repeating values and then open another issue for the UI and form submissions in general for cockpit and develop a standard on how/when we should validate all form submissions platform wide and start working on implementing that. thoughts?
Also just a side note I did search patternfly pretty extensively and I did not see anything related to how/when to display error messages back to the user related to form submission. Could have easily missed it though there is quite a lot going on there. @garrett may be better at pointing me in the right direction. Further the form validation section in patternfly is based on state changes which if they get out of sync with the current DOM or are undefined allows for bad form submission. that's why I opted to use traditional html/javascript form validation checks in combination with state hooks.
as for time to Implement I don't think it would be that bad. Less then a day with testing but I think I need the direction on how to proceed with notification, disabling the save button, when we validate etc.
@dphaas2004: PatternFly has a section on error messages on the form component design page @ https://www.patternfly.org/v4/components/form/design-guidelines/#errors-and-validation:
When a form field submission results in an error, let users know as soon as possible and as close as possible to the error. Default to presenting error states on a form using field level errors whenever possible.
In some use cases, you may choose to use inline errors at the top of a form to emphasize errors within it. These inline errors should always be a supplemental component used in addition to field level errors.
(Screenshot above shows an inline error at the bottom of the form. It should be at the top, and this should be fixed... either in this PR or in a follow-up. Both are fine, as long as it's addressed at some point soon.)
Then it has additional text, including multiple sub-sections, such as:
For in-modal forms in Cockpit, they're checked on the fly during entering information (sometimes), on focus blur (usually, when possible), and on submission (always). In other words, we usually try to check things as soon as possible and always check it during the submission process.
@marusak just getting some time to jump back into this. I believe I have most all cleaned up. @garrett Thanks for the links. I have moved the error messages to a new header in the modal and added a statehook for the form to track its validation. the submit button validation is conditional on the form being valid now. Please take a look when you get a moment.
only lingering issue I am working on is related to the datepicker. I see this is a beta feature in patternfly. There appears to be an open bug on the datepicker validators. any reason why we cant use a standard date input field for this?
@dphaas2004: The native date input has issues, sadly. From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/date#handling_browser_support:
The second problem is the more serious one; with date input supported, the value is normalized to the format yyyy-mm-dd. But with a text input, the browser has no recognition of what format the date should be in, and there are many different formats in which people write dates.
...
At the moment, the best way to deal with dates in forms in a cross-browser way is to have the user enter the day, month, and year in separate controls, or to use a JavaScript library such as jQuery date picker.
It's a bummer, but that's how it is. This is why PatternFly has their own widget for it.
@garrett not really a cockpit issue but im coming up short with the patternfly datepicker. Is it used elsewhere in cockpit? how are they validating? Im noticing that the onBlur event on the datePicker doesnt fire if a date is actually picked? This is causing some funky behavior. Also using the validator is messy as well as it appears to evaluate realtime which puts quite a load on things and eventually kicks out on a maximum depth exceeded error as I call setState too many times. Pretty messy but I think I have it working reasonably well now with exception of the blur issue. My only thought on correcting that issue is using a synthetic event onchange to trigger the blur but wondering if this is an issue with datepicker or intended result? I dont know enough about datepicker to say.
~Also appears to be an issue with the datepicker itself in the current base which causes the modal div to get very large when you try to select a date. Please let me know if I should open a separate issue for this.~ I see you noticed this same issue.
If PatternFly's isn't working, then we can use the native one, try to be aware of its shortcoming(s), and test cross-browser (in latest Firefox, Chrome, WebKit). And then we can file bugs upstream to PF.
Generally, I'm a fan of native widgets when possible anyway.
@jelly yes there was (and still are some) some issues specifically with validation in the repeating loop section of the form. I have made many improvements to this but its still now quite right. ill push my current code that should take care of your concern. Im open to any suggestions on how to get the btn disabled state to work properly with the repeating validation. that is the last missing piece.
@jelly yes there was (and still are some) some issues specifically with validation in the repeating loop section of the form. I have made many improvements to this but its still now quite right. ill push my current code that should take care of your concern. Im open to any suggestions on how to get the btn disabled state to work properly with the repeating validation. that is the last missing piece.
If you can wait, I'm going to push a rebased PR and take a further look.
yep I can wait. Also just a side note im pretty positive in the current version the form submission will crash. I have also fixed this.
yep I can wait. Also just a side note im pretty positive in the current version the form submission will crash. I have also fixed this.
It indeed crashes, locally you should be able to git pull --rebase now.
@dphaas2004 hi :) Wanted to check if you still want to work on this or if someone from the cockpit team should pick it up for you.
