do some validation on filters before updating the map #355
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #355 +/- ##
==========================================
+ Coverage 82.24% 82.30% +0.06%
==========================================
Files 87 87
Lines 2636 2645 +9
Branches 275 276 +1
==========================================
+ Hits 2168 2177 +9
Misses 404 404
Partials 64 64 ☔ View full report in Codecov by Sentry. |
struan
force-pushed
the
328-null-filter-error
branch
from
c3c5dac to
5e6297d
Compare
There was a problem hiding this comment.
Generally this seems good to me, and it works locally, so I guess we could merge and deploy.
But I’ve left comments with one suggestion on simplified javascript, and some pondering of the best time to display the is-invalid styling on the inputs. I wonder whether you have any thoughts?
| @@ -245,9 +245,21 @@ const app = createApp({ | |||
|
|
|||
| return url | |||
| }, | |||
| filtersValid() { | |||
There was a problem hiding this comment.
@struan You could use Array.prototype.every for this, which is a bit simpler to follow, I think:
filtersValid() {
return this.filters.every(function(filter){
return filter.selectedValue !== "";
});
}| <input v-else-if="filter.data_type == 'integer'" type="text" inputmode="numeric" pattern="[0-9]*" v-model="filter.selectedValue" class="form-control form-control-sm flex-grow-1 flex-shrink-1"> | ||
| <input v-else type="text" v-model="filter.selectedValue" class="form-control form-control-sm flex-grow-1 flex-shrink-1"> | ||
| <input v-else-if="filter.data_type == 'integer'" type="text" inputmode="numeric" pattern="[0-9]*" v-model="filter.selectedValue" class="form-control form-control-sm flex-grow-1 flex-shrink-1" :class="{ 'is-invalid': filter.selectedValue === '' }"> | ||
| <input v-else type="text" v-model="filter.selectedValue" class="form-control form-control-sm flex-grow-1 flex-shrink-1" :class="{ 'is-invalid': filter.selectedValue === '' }"> |
There was a problem hiding this comment.
Not sure whether this is what you intended, but it appears this is-invalid class is only applied after the input has been edited at least once before. ie: when the input is brand new (and, in most cases, empty), it’s not marked as is-invalid, but if you enter some text, then delete it, the input is correctly marked as is-invalid.
TBH this actually feels ok to me – always feels nasty immediately displaying an empty input as invalid, before the user’s had a chance to modify it. But I guess it begs the question – why display invalid classes at all, if there are cases where an empty input might sneak through regardless. 🤷
There was a problem hiding this comment.
I guess an alternative (although, with more complex JavaScript) would be to only mark the inputs as invalid after the user has pressed the "Update map" / "Update table" button.
At least then you’d also get some interactive feedback when you press the button and nothing happens. Right now, as it is, if you miss (or misunderstand) the invalid styling, it just feels like the button is broken and not responding to clicks.
There was a problem hiding this comment.
In theory everything has a default value so we should never show an empty box initially.
There was a problem hiding this comment.
The filter for "MP positions (job titles)" just presents a free text input, and doesn’t have a default value.
It looks like "MP Select Committee memberships" also displays a free text input – but TBH I think that’s a bug, and it should actually be displaying a <select multiple> like the "MP APPG memberships" dataset.
There was a problem hiding this comment.
oh yes, those. As you say though I think it's fine. FWIW, I think the select committee one is ok text. There's 50 odd committees at the moment which is a fairly unwieldy dropdown.
There was a problem hiding this comment.
Yeah, a free text match on committee name makes sense, but not when it’s prefixed by "is one of" and followed by the ctrl-click/cmd-click help text (both of which should only be displayed next to a <select multiple>)…
Anyway, that feels like a separate ticket though.
struan
force-pushed
the
328-null-filter-error
branch
from
5e6297d to
9ebd96a
Compare
Highlight empty filters and also prevent the map updating if there are empty filters. Also ignore empty filters at the back end to avoid 500 errors Fixes #328
struan
force-pushed
the
328-null-filter-error
branch
from
9ebd96a to
7d0ec5d
Compare
Highlight empty filters and also prevent the map updating if there are empty filters.
Also ignore empty filters at the back end to avoid 500 errors
Fixes #328