-
Notifications
You must be signed in to change notification settings - Fork 31
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 date range filters to the search UI #2081
Conversation
This will be used for the date range filters for BigCZ catalog search.
+1 for the UX decisions described in the Notes.
It sounds like a UI element like a flag or highlight of some kind would be useful to show that a filter is activated, even when the filter is hidden. We should address this in a design card. /cc @jfrankl |
src/mmw/js/src/data_catalog/views.js
Outdated
}, | ||
|
||
onRender: function() { | ||
$('.data-catalog-date-input').datepicker(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to import $
before we can use it. That's causing the linting errors:
$ kj npm run lint
js/src/data_catalog/views.js: line 151, col 9, '$' is not defined.
js/src/data_catalog/views.js: line 170, col 27, '$' is not defined.
2 errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the code, looks well organized and straightforward. Going to run it now.
src/mmw/js/src/data_catalog/views.js
Outdated
model: new modalModels.AlertModel({ | ||
alertMessage: "The bounding box of the current area of " + | ||
"interest is " + Math.round(area) + " km², " + | ||
"interest is " + formattedArea + " km², " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon:  km
→ km
src/mmw/js/src/data_catalog/views.js
Outdated
"which is larger than the current maximum " + | ||
"area of " + MAX_AREA_SQKM + " km² supported " + | ||
"for WDC.", | ||
"area of " + MAX_AREA_FORMATTED + " km² " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too.
@@ -17,6 +17,8 @@ var Catalog = Backbone.Model.extend({ | |||
id: '', | |||
name: '', | |||
description: '', | |||
dateStart: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider changing to fromDate
and toDate
to match the API.
@@ -44,6 +44,11 @@ | |||
"from": "https://registry.npmjs.org/bootstrap/-/bootstrap-3.3.4.tgz", | |||
"resolved": "https://registry.npmjs.org/bootstrap/-/bootstrap-3.3.4.tgz" | |||
}, | |||
"bootstrap-datepicker": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 I like the neatness of the shrinkwrap changes! There is a typo in the commit message though javascrip
→ javascript
.
src/mmw/js/src/data_catalog/views.js
Outdated
after = moment(this.model.get('dateEnd'), dateFormat), | ||
isValid = false; | ||
|
||
if (!before || !after) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the validation message is behaving strangely in your screenshot, I'll revisit and make sure I didn't alter something as I was cleaning up my commits.
<input | ||
class="data-catalog-date-input form-control" | ||
data-date-range="start" | ||
value="{{dateStart}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I found the labels problematic to map to the API attributes. I'll assume they were determined casually and come up with something that can be better understood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take another look once the above issues have been addressed.
A few thoughts: I prefer when the validation message is on its own line below the inputs. In some screen captures, it is displayed to the right of the inputs. Would be good if there were a clear button to remove the date when one has been entered. The wireframes show a "X Reset" button after a date has been entered. An "X" button inside the inputs, on the right side, would also work. Other than that, this looks good to me, and once it's in I can do some minor style cleanup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 tested. All cases work well, code looks great. Nice work!
Adds UI elements for collecting start and end date filters and passes them along to the existing API endpoint using the pre-exisitng query string parameters. Does some light validation to ensure that before < after, which would otherwise raise exceptions on the backend.
Uses formatted numbers and non-breaking spaces between the value and units.
The h2 element with the Data Source title on the search page had its size restricted by these css rules. Removing them allows the full element to be shown, as in other sidebar pages.
When a date filter is given a value, a clear icon is added to the element for easy removal.
c5ed65a
to
fc32018
Compare
@rajadain Thanks for the review. I squashed my fixups, and added a new commit addressing @jfrankl's comment on the "clear" buttons. These weren't in the wireframes posted in the issue, but I interpreted it as: They only show up when there is a value in the box, and clicking it will clear out the value. Adding this in created some unwanted visual effects (the box grows and shrinks if they are present/absent, and it's no longer flush with the edge). I attempted to restore my previous style, but think the effort would be better spent during the design pass. Could you give it one more spin? |
Taking another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 tested the new stuff, works well!
Overview
Adds UI elements for collecting start and end date filters and passes them along to the existing API endpoint using the pre-existing querystring parameters. Does some light validation to ensure that
before < after
, which would otherwise raise exceptions on the backend.Also adds a small formatting improvement to the warning dialog for areas which are too large for WDC to handle.
Connects #1933
Demo
Added filter toggle
Filters exposed with date-picker
Validation response
Warning - before
Warning - after
Notes
I uncovered an issue that is visible here, but also on staging (though less pronounced), captured here: #2080. I also made a good-faith effort to match the styling shown in screenshots, but this could use some touch-ups as part of a final style pass on the larger feature.
I had to derive a bit of UX from the screenshot listed in the issue. My decisions on the behavior are as followed, and are open to feedback:
Testing Instructions