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

Chart Form Validation #41

Merged
merged 2 commits into from
Jul 10, 2018
Merged

Chart Form Validation #41

merged 2 commits into from
Jul 10, 2018

Conversation

heathharrelson
Copy link
Member

Validate chart form when inputs change and prior to saving.

Validate chart form when inputs change and prior to saving.
<span class="input-group-addon" @click="showDatePicker($refs.targetEndDateInput)">
<span class="glyphicon glyphicon-calendar"></span>
</span>
</div>
<div class="help-block error-help">{{ targetEndDateErrors }}</div>
<div class="help-block error-help">{{ errors.endError }}</div>
</div>
</fieldset>

Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes invalid fields aren't visible on the screen. This indicates why the save button is disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, that's definitely an improvement.

Trim input values before validating them to match the behavior
of `v-model`.
*/
validateForm() {
const { $el } = this;
const inputs = $el.querySelectorAll(selector.validatedInputs);
Copy link
Member Author

Choose a reason for hiding this comment

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

According to MDN, not all browsers support using forEach on a NodeList, hence the for loop.

@@ -179,6 +187,8 @@ import {
fieldLabel
} from '@/util';

const userDateFormat = 'MM/DD/YYYY';
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to deal with this now, but REDCap allows users to dictate their preferred date format. I'm not sure if this is available to us via the SDK, but it might be useful at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good point. Right now I'm just trying to mimic the behavior of the jQuery version, so I opened issue #45 to track this.

Copy link
Contributor

@lawhead lawhead left a comment

Choose a reason for hiding this comment

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

Seems like this is pretty similar to the previous behavior, but cleaned up a bit.

I like that it's declarative, but sometimes the logic gets confusing when we are validating a single field at a time, but that field has several dependencies (for instance, the validateTargetDate method). I wonder if a hybrid approach would simplify this. Keep the simple checks like formatting and required elements declarative, but then have a separate check for the more complicated model-level rules.

@heathharrelson
Copy link
Member Author

heathharrelson commented Jul 10, 2018

@lawhead I'm not 100% happy with how this came out, and I agree that it could use another round of refactoring. I also think it would be worthwhile to try a validation library like VeeValidate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants