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

add property for disabling validation on visibility change #399

Merged
merged 3 commits into from
Jun 6, 2017

Conversation

PoplarToppler
Copy link
Contributor

This project uses semver, please check the scope of this pr:

  • #none# - documentation fixes and/or test additions
  • #patch# - backwards-compatible bug fix
  • #minor# - adding functionality in a backwards-compatible manner
  • #major# - incompatible API change

The motivation behind this is that when using the form for facets with validation, I am using onValidation to trigger the server request so that I am able to block invalid filters (e.g. invalid date range) while still allowing the query to succeed.

The problem is that if we want to freeze the current view (e.g. stop polling), then go to another tab (e.g. new details tab) then return to the original view, a new query is triggered due to the validation of the form being triggered on visibility change. This ends up changing the view that was supposed to be frozen.

CHANGELOG

  • Added validateOnVisibilityChange property to allow disabling of form validation after losing/regaining focus of the page
  • Fixed select documentation formatting

@PoplarToppler
Copy link
Contributor Author

PoplarToppler commented May 2, 2017

Let me know if you would like me to add any tests around this. I didn't see any existing ones so I wasn't sure where to put them if I should add any. Assuming you are ok with the change.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7c49088 on PoplarToppler:master into ** on ciena-frost:master**.

@sandersky
Copy link
Contributor

@PoplarToppler it'd be nice to see some tests, even if it is just a few unit tests against the _onVisiblityChange method for the various cases (probably 3 tests in total).

@PoplarToppler
Copy link
Contributor Author

Sorry I have been super busy. I will get back to and finish this soon.

@PoplarToppler
Copy link
Contributor Author

I'm not sure where I should put these tests, @job13er can you give me a recommendation? Thanks!

@job13er
Copy link
Contributor

job13er commented Jun 1, 2017

I'm not really familiar with the testing here. I'd defer to @agonza40 or @sophypal.

@job13er
Copy link
Contributor

job13er commented Jun 1, 2017

I've also pretty much stopped using bunsen now as well (no more data driven requirements) so I'm not sure I'm the best person to review this. Do we have someone who is going to step up as primary maintainer with Matt gone?

@sglanzer-deprecated
Copy link
Contributor

We will, but we're not in a position to do it until later in June

@PoplarToppler
Copy link
Contributor Author

@agonza40 @sophypal do either of you have suggestions of where I could put a few tests for this new property? I'd ideally like to get this in soon. My best guess is I'd add a new file: tests/unit/components/frost-bunsen-form/on-visibility-change-test.js and go from there. Does that sound acceptable?

@sophypal
Copy link
Contributor

sophypal commented Jun 1, 2017

@PoplarToppler Typically we model the test directory against the src directory. Since there is very little unit test (due to favoring integration in this project), you'd need to add tests/unit/components/frost-bunsen-form/component-test.js and have your scenario in it's own describe block.

onChange () {},
onError () {}
})
component.init()
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to call init here as this.subject calls it indirectly through instantiation.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 70fe0d0 on PoplarToppler:master into ** on ciena-frost:master**.

@sophypal
Copy link
Contributor

sophypal commented Jun 6, 2017

👍

Approved with PullApprove

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 238d746 on PoplarToppler:master into ** on ciena-frost:master**.

@PoplarToppler PoplarToppler merged commit f32d917 into ciena-frost:master Jun 6, 2017
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.

7 participants