-
Notifications
You must be signed in to change notification settings - Fork 53
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
Wizard: Validate snapshot date with useValidation #2305
Conversation
I'm drafting this as the custom sub nav item is broken with the error state and the date component is not easy to use the same validation function, but that's kinda the main point, so bit useless without it. |
7711ccb
to
fcaab27
Compare
fcaab27
to
13ea867
Compare
it is look good except one thing, when user go to 'Repository snapshot' step to fix the problem, the error message 'Cannot set a date in the future' doesnt show up Screen.Recording.2024-08-06.at.11.35.10.movand I like the changes that the red symbol appear align with the step name |
I don't see this as an issue. If the user really wants to mess around then they shouldn't be surprised by an extra line break here and there 😈 |
Noticed the same issue. I think it's connected to the pristinity of the date picker maybe? If I mess up, go to Review, then jump back to Snapshots the helper error text doesn't show unless I click on the date picker and away again. Otherwise looks great! |
Hmm the problem is, the pristine is not under our control :/ I'm not sure if I'm able to fix it 🤔 |
13ea867
to
05abf99
Compare
I see, sorry didn't realize the pristinity is out of our control here 🤔 I'd say in the codebox it works the same way as it does in the PR. When you're manipulating data in the date picker "manually" it doesn't show the helper text unless you click outside of the date picker Which means we're consistent with PatternFly patterns so I'd say all's fine 🤷 And migrating the validation to use |
05abf99
to
01380f2
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.
Thank you!
Wait, I'm trying to think which is a bit hard today I must admit so please bear with me 😅 Could we use the same style of validation we use in FSC custom footer? Meaning the Next button would stay enabled, but the validation runs upon clicking it and it gets disabled when the validation errors, canceling the pristinity and rendering the helper text? I'm asking because the same problem applies to #2374 I think this could be resolved in a follow up as it's a bit different problem than just migrating snapshot validation to |
01380f2
to
c769137
Compare
/retest |
1 similar comment
/retest |
I guess it makes sense 🤔 I don't love what we do there (make you click to see the validation = for me it feels like a bait button), it surfaces the fact of delayed validation more clearly. |
I see, also good point 🤔 the thing I'm trying to figure out is which way of validating is better. Either we block the Next button, but don't tell the user why unless they start clicking around the Wizard. Or we allow Next button, but disable it after a click and immediately show errors if there are some. I'm not sure which method is better. 😅 I think the second approach is more transparent in the way the error helper texts are handled, but yeah, the enabled Next button which is not really enabled... hmm. Well I still think we can merge this PR and continue discussing the problem in #2392 |
c769137
to
0d2280a
Compare
This uses the concept of useValidation for snapshot date.
It is the last validation that doesn't use this.