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: Add beforeNext conditions to Details step footer #2392

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

Conversation

regexowl
Copy link
Collaborator

This adds beforeNext condition to the custom Details step footer, allowing to validate after clicking Next. That way the button is enabled, but gets disabled in the case validation errors and the helper text with the error renders immediately.

@regexowl
Copy link
Collaborator Author

regexowl commented Aug 29, 2024

@ezr-ondrej This is what I had in mind in #2305 (comment), what do you think? Would this make sense?

edit: to also add some arguments - this would be consistent with the FSC step, so it's a pattern we already use. The error wouldn't jump at the user prematurely, but at the time when they're happy with how they filled up the step and would like to continue. And this should also be compatible with the "always blue" strategy of Next button being always enabled unless there's a problem that need to be resolved.

@mgold1234
Copy link
Collaborator

looks good, just small suggestion

@regexowl
Copy link
Collaborator Author

regexowl commented Sep 3, 2024

To continue the discussion we had with @ezr-ondrej #2305 (comment) - does this make sense or not for our use case?

@lucasgarfield @mgold1234 With the way things are set up in PatternFly validation we have two options of showing the user validation errors. This will be specifically for the Details step's Blueprint name input, but there are several places where similar problem occurs (snapshot validation comes to mind):

  1. when a non-valid name is entered, we currently disable Next, but an error doesn't get rendered unless the user clicks outside of the input (onBlur). That means user can have the name filled in, can't continue because of disabled button, but doesn't see why.
  2. similarly to FSC step we could let the Next button be enabled, but disable it and render an error when the Next button with non-valid name filled in is clicked, which would be this PR.

A fair point is that the always enabled Next even with incorrect values might feel like a bait. On the other hand I feel it's a bit more transparent, but to be honest I'm very much undecided on this. What do you think?

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.68%. Comparing base (1a7350e) to head (a4fdde9).
Report is 2151 commits behind head on main.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2392      +/-   ##
==========================================
+ Coverage   75.71%   83.68%   +7.97%     
==========================================
  Files          33      153     +120     
  Lines         597    17243   +16646     
  Branches      144     1678    +1534     
==========================================
+ Hits          452    14430   +13978     
- Misses        139     2794    +2655     
- Partials        6       19      +13     
Files with missing lines Coverage Δ
...Components/CreateImageWizard/CreateImageWizard.tsx 99.42% <100.00%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34bdfa3...a4fdde9. Read the comment docs.

@regexowl regexowl force-pushed the pristine-details-validation branch 2 times, most recently from da04578 to dd717e9 Compare September 19, 2024 09:19
This adds beforeNext condition to the custom Details step footer, allowing to validate after clicking Next. That way the button is enabled, but gets disabled in the case validation errors and the helper text with the error renders immediately.
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.

2 participants