Skip to content

skip js validation for readonly/disabled fields#1306

Closed
Liviu-p wants to merge 1 commit into
masterfrom
issue-4447
Closed

skip js validation for readonly/disabled fields#1306
Liviu-p wants to merge 1 commit into
masterfrom
issue-4447

Conversation

@Liviu-p
Copy link
Copy Markdown
Contributor

@Liviu-p Liviu-p commented Sep 28, 2023

@Liviu-p Liviu-p requested a review from Crabcyborg September 28, 2023 18:41
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.22%. Comparing base (dc13710) to head (31336ab).
Report is 3187 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1306      +/-   ##
============================================
+ Coverage     29.05%   29.22%   +0.17%     
- Complexity     7217     7233      +16     
============================================
  Files           104      105       +1     
  Lines         23808    23853      +45     
============================================
+ Hits           6917     6972      +55     
+ Misses        16891    16881      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@garretlaxton
Copy link
Copy Markdown

Should I still get a error when the time field is blank even though it's read-only?
image

@Liviu-p
Copy link
Copy Markdown
Contributor Author

Liviu-p commented Oct 3, 2023

Should I still get a error when the time field is blank even though it's read-only? image

I'm not sure about this, it makes sense to me to trigger the error as long it is set up as required + readonly. @Crabcyborg what do you think?

@Crabcyborg
Copy link
Copy Markdown
Contributor

Thanks @Liviu-p and @garretlaxton. This seems like a weird conflict of settings the more I think about this.

I don't understand how something would make sense as read only, and also required, with no default value. It seems like something that should display a warning in the form builder.

@stephywells What do you think makes sense here? Should we really validate anything disabled with JS? When it's disabled, no value gets sent anyway so I don't know why we'd validate it before its sent.

@stephywells
Copy link
Copy Markdown
Contributor

@Crabcyborg it seems like a validation message isn't helpful if the user can't do anything about it. But we also don't want to allow people to manually clear out the read only value and then get past validation.

This does seem to me like more of an error on the builder end that we should notify of the issue. Is it possible to handle this type of error with a message on the builder page like you mentioned?

@Crabcyborg
Copy link
Copy Markdown
Contributor

Thanks @stephywells! We'll look into adding a warning.

@Crabcyborg
Copy link
Copy Markdown
Contributor

@Liviu-p Could you try to work on that when you have the chance?

@Liviu-p
Copy link
Copy Markdown
Contributor Author

Liviu-p commented Oct 11, 2023

@Liviu-p Could you try to work on that when you have the chance?

sure @Crabcyborg, I will take care of it.

@Crabcyborg Crabcyborg requested review from Crabcyborg and removed request for Crabcyborg and garretlaxton October 17, 2023 20:16
@Crabcyborg Crabcyborg marked this pull request as draft October 17, 2023 20:17
@Crabcyborg
Copy link
Copy Markdown
Contributor

I'm going to close this PR for now.

I think we can leave the validation as-is for now.

I fixed this issue in another way (See #1946), by addressing the JS error instead.

@Crabcyborg Crabcyborg closed this Aug 23, 2024
@Crabcyborg Crabcyborg deleted the issue-4447 branch August 23, 2024 16:23
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.

4 participants