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

Wizard: Validate snapshot date with useValidation #2305

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

ezr-ondrej
Copy link
Collaborator

This uses the concept of useValidation for snapshot date.
It is the last validation that doesn't use this.

@ezr-ondrej
Copy link
Collaborator Author

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.

@ezr-ondrej
Copy link
Collaborator Author

Well it's not broken, just the text is too long to fit the status 😮‍💨

image

We could change the text and it works 😛
image

Or accept it's "broken" - we do not usually show this to User, as they cannot go next on invalid step, so they need to hack it around 🤷

@ezr-ondrej ezr-ondrej marked this pull request as ready for review August 5, 2024 18:58
@mgold1234
Copy link
Collaborator

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

and I like the changes that the red symbol appear align with the step name

@regexowl
Copy link
Collaborator

regexowl commented Aug 6, 2024

Or accept it's "broken" - we do not usually show this to User, as they cannot go next on invalid step, so they need to hack it around 🤷

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 😈

@regexowl
Copy link
Collaborator

regexowl commented Aug 6, 2024

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

and I like the changes that the red symbol appear align with the step name

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.

snapshot-validation

Otherwise looks great!

@ezr-ondrej
Copy link
Collaborator Author

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.

Hmm the problem is, the pristine is not under our control :/
I've tried to report it as a bug to PF, but in a codebox it works as expected :(

I'm not sure if I'm able to fix it 🤔

@regexowl
Copy link
Collaborator

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

helper-text

Which means we're consistent with PatternFly patterns so I'd say all's fine 🤷 And migrating the validation to use useValidation is an improvement for sure so let's do this! 🚀 Thanks a lot for looking into it!

Copy link
Collaborator

@regexowl regexowl left a comment

Choose a reason for hiding this comment

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

Thank you!

@regexowl regexowl enabled auto-merge (rebase) August 29, 2024 12:32
@regexowl regexowl disabled auto-merge August 29, 2024 12:42
@regexowl
Copy link
Collaborator

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
The Next gets disabled with a duplicate name, but the helper text doesn't render until blur.

I think this could be resolved in a follow up as it's a bit different problem than just migrating snapshot validation to useValidation.

@regexowl
Copy link
Collaborator

/retest

1 similar comment
@regexowl
Copy link
Collaborator

/retest

@ezr-ondrej
Copy link
Collaborator Author

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

@regexowl
Copy link
Collaborator

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

@regexowl regexowl merged commit 2cf95be into osbuild:main Aug 30, 2024
5 checks passed
@ezr-ondrej ezr-ondrej deleted the snapshot_use_validation branch September 1, 2024 22:47
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.

3 participants