Skip to content
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 Minutely and Custom options to specific time trigger for creating timers #17592

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dphaas2004
Copy link
Contributor

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.

@dphaas2004 dphaas2004 temporarily deployed to cockpit-dist July 25, 2022 15:53 Inactive
@marusak
Copy link
Member

marusak commented Jul 26, 2022

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:
    Screenshot from 2022-07-26 09-42-32

  • Minutely is all good! It needs test though. See for example here: https://github.com/cockpit-project/cockpit/blob/main/test/verify/check-system-services#L1149

  • Custom is 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.

@marusak
Copy link
Member

marusak commented Jul 26, 2022

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.

@marusak
Copy link
Member

marusak commented Jul 26, 2022

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 #17595

@dphaas2004 dphaas2004 temporarily deployed to cockpit-dist July 26, 2022 12:28 Inactive
@dphaas2004 dphaas2004 temporarily deployed to cockpit-dist July 26, 2022 12:46 Inactive
@dphaas2004
Copy link
Contributor Author

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?

@dphaas2004 dphaas2004 temporarily deployed to cockpit-dist July 26, 2022 15:21 Inactive
Copy link
Member

@garrett garrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good.


I also see the dropdown issue. I'll be glad when that's fixed.

I think this is outside the scope of this PR as well, but, since we're here: if specific time is more commonly used and is default, why is it second? We should reorder.

Copy link
Member

@marusak marusak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you for the fixes! Some comments and also for @garrett about design

pkg/systemd/services/timer-dialog.jsx Show resolved Hide resolved
pkg/systemd/services/timer-dialog.jsx Outdated Show resolved Hide resolved
pkg/systemd/services/timer-dialog.jsx Outdated Show resolved Hide resolved
pkg/systemd/services/timer-dialog.jsx Outdated Show resolved Hide resolved
pkg/systemd/services/timer-dialog.jsx Outdated Show resolved Hide resolved
pkg/systemd/services/timers.scss Outdated Show resolved Hide resolved
test/verify/check-system-services Outdated Show resolved Hide resolved
Copy link
Member

@garrett garrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops! I missed the "custom" part of this PR. I'll write what needs to change inline.

@dphaas2004 dphaas2004 temporarily deployed to cockpit-dist July 27, 2022 12:27 Inactive
@dphaas2004 dphaas2004 temporarily deployed to cockpit-dist July 27, 2022 12:39 Inactive
Copy link
Contributor Author

@dphaas2004 dphaas2004 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should all be cleaned up now with adding helpertext and removing the custom css

@marusak
Copy link
Member

marusak commented Aug 4, 2022

Current screenshot for reference:
Screenshot from 2022-08-04 09-34-36

@garrett can you please ack this? Thanks!

@marusak
Copy link
Member

marusak commented Aug 4, 2022

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!

Copy link
Member

@garrett garrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper text on a form is supposed to be closer to the entry.

https://www.patternfly.org/v4/components/form#horizontal

The link isn't supposed to be on its own line. The link should be modifying the text, not the example. I tried a version with the example in the same line, but it's a bit much, so I guess having the example as a placeholder (as guidance) and linking to the reference is probably more correct in this case, like this:

timer-mod2

@garrett
Copy link
Member

garrett commented Aug 4, 2022

The problem with the spacing is because of this:

image

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.

@dphaas2004 dphaas2004 temporarily deployed to cockpit-dist August 5, 2022 15:22 Inactive
@dphaas2004 dphaas2004 temporarily deployed to cockpit-dist August 5, 2022 15:46 Inactive
@dphaas2004
Copy link
Contributor Author

@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!

@marusak marusak temporarily deployed to cockpit-dist August 8, 2022 08:31 Inactive
@dphaas2004
Copy link
Contributor Author

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.

@dphaas2004
Copy link
Contributor Author

@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.

@dphaas2004
Copy link
Contributor Author

Fixed repeat validation and rebased.

@marusak
Copy link
Member

marusak commented Aug 31, 2022

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.

@dphaas2004
Copy link
Contributor Author

@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.

@dphaas2004
Copy link
Contributor Author

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.

@garrett
Copy link
Member

garrett commented Sep 8, 2022

@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.

@dphaas2004
Copy link
Contributor Author

@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?

@garrett
Copy link
Member

garrett commented Sep 26, 2022

@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
garrett previously approved these changes Sep 26, 2022
Copy link
Member

@garrett garrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall: looks good. 👍

I spotted a problem with yearly:

image

I don't think it's related to this PR, however. I'll open up a separate issue for it.

@dphaas2004
Copy link
Contributor Author

@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.

@garrett
Copy link
Member

garrett commented Sep 27, 2022

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
Copy link
Member

jelly commented Oct 24, 2022

Regression with yearly:

image

@dphaas2004
Copy link
Contributor Author

@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
Copy link
Member

jelly commented Oct 24, 2022

@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.

@dphaas2004
Copy link
Contributor Author

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.

@jelly
Copy link
Member

jelly commented Oct 24, 2022

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.

@jelly jelly temporarily deployed to cockpit-dist October 24, 2022 13:54 Inactive
@KKoukiou
Copy link
Contributor

@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.

@dphaas2004
Copy link
Contributor Author

@KKoukiou, If someone at cockpit could take a look at the validation routine for the repeating loops as I suggested to @jelly that would be great. I'm tied up with another project implementation so wont be able to get back on this for a while.

Added minutely to Create Timer Modal

Added option for Minutely Repeating Timer.

Add Custom OnCalendar Option to specific time Trigger

Added Custom option to specific time option.
Fixed UserInputs errors on TextInput requiring ID or aria-labels.

Added System Test for Minutely

Added test for custom input

Each custom input would be different but this test the basic concept

Remove un-needed variable declarations

Added Popover for details on custom input and some css for textinput

Removed css, added helper text

Removed hard coded css sizing
Removed class from textinput
Removed Popover in lieu of helpertext
Added external link icon
changed external link
Swapped timer order per cockpit-project#17598

Removed redundant call.

Removed wait_visible as click already does it

Cleaned up HelperText

Added Minutely to specific-time Create Timer Modal

Added option for Minutely Repeating Timer.

Added System Test for Minutely

services: Added minutely and custom options to timer creation

Fixed form validation

form validation was not happening until the form was submitted or was happening onchange causing a resource issue.  further, the existing onsubmit function was not properly validating the form inputs before passing on to the spawn event and could be triggered with a partially complete form.

new validate function validates inputs as needed and keeps state.  further the save button is disabled until key states exist.

added Yearly repeat validation as it was missing.

updates to onsubmit properly checks validation state for each use case.

replaced validationFailed with state object hook.
removed submitted state hook as it was not needed

Added isRequired to form inputs

fixed error in validate function
added isRequired to fields
added name to fields (for proper FormData)
updated onsubmit function to do more complete form validation checks.

fixed validation for repeating values

-added validation onBlur and helpertext keys to the array of objects repeatPatterns state hook for tracking validation.
-Use IsRequired to confirm validation on submit.
-use input pattern for validation.
-yearly is validated separately as there is no IsRequired for DatePicker.
-daily, weekly, monthly are not validated as they are drop down menus with default options.

added better formvalidation

Updated the submit function
added a validFrom state hook to the submit button
added a modal header
added a check for error message on validation to clear it out if the form changes.

better datepicker validation

cleaned up the datepicker validation best i could.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants