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

WIP 317 Add a date filter element #385

Closed
wants to merge 6 commits into from
Closed

WIP 317 Add a date filter element #385

wants to merge 6 commits into from

Conversation

ptgott
Copy link
Collaborator

@ptgott ptgott commented Jul 8, 2017

Addresses #317

@NealHumphrey

I've gone with a noUISlider element for controlling the date filter. This is to allow the user to set a rough date quickly and get a broad sense of which subsidies start/end when. I'm also thinking about adding an 'enter precise dates' link below the slider that swaps the slider for a set of text inputs (for month, day and year for min and max values).

In order to compare the dates of different 'items' in dataCollection.filterData, the code iterates through 'items', takes all the values of properties corresponding to 'date' components, and replaces them with JavaScript Date objects.

Currently the filtering is broken: While the date slider sets the app state with new Date values successfully, this doesn't yet do any actual filtering. We can check whether the units that are displayed by default have 'null' poa_start or poa_end values -- that might be one cause. Another could be the operation used to compare values of the filter element with values of the data. While comparison operators work with JS Date objects, I'm not sure if this is what's being used.

A side note: I've changed the file name of one of the images in the 'assets' folder. This is because the length of the name was preventing the Jekyll web server from running on my laptop. I'm pretty sure my use of OneDrive and Windows 10 is to blame, given OneDrive's restrictions on filenames.

… JS Dates. Create a slider element for filtering dates. The slider element sets the state to a new minimum and maximum date, with dates given as JS Date objects.
@NealHumphrey
Copy link
Collaborator

@ptgott Sounds like a good start.

First off - this is my fault for not realizing the conflict when I suggested this task. You should make this branch off of the 'demo-v2-options-combined' branch instead of dev. On Thursday we decided to go ahead with this approach; since this merges the layer+filter functionality, there are some refactored changes to how these work (mostly one universal function for adding title text/formatting to the title of each filter control). It's probably easier to make a new branch and move your changes manually rather than trying to rebase or use git merges.

Whenever you're going to do work on this let me know - I'm planning to finalize the changes in demo-v2 today+tomorrow so that it can be merged into dev, which will also involve a bit of refactoring of how functions are organized between the filterView and mapView docs currently - hopefully I can get that done before you're ready to move the code over to be on top of demo-v2, which would be cleanest, but if you want to do any work on it before I'm done just let me know so I can make sure not to do anything that will interfere.

As you decide how to finalize this, we should think about how it overlaps with the other noUiSlider as well. In particular, as you observe we definitely want a way to type in the specific min/max for both dates and continuous values. We saw this in user testing - while users liked the sliders for rough use as you said, eventually everyone realized they also wanted precision.

I think the most useful way to do this might be to have min and max form fields below the slider. We could eliminate the slider tooltip box and instead have the slider update the value of the form field if the user moves the slider. Similarly, when the user changes the typed in value the slider could update (there's already code to do this when clearing filters) - probably triggered on the onfocusout event.

On a related note, what do you think about maybe having the slider change one year at a time when you drag it? It could always be set to 1/1/YYYY. That might make the slider more useful - if you want to just set it based on the year, you could just drag, as opposed to dragging and then realizing you still need to set the specific date.

On your question about filtering not working, this is because the file /utils/filter.js has this code in it for handling dates:

if (component['component_type']== 'date') {
    //TODO determine whether dates will always be published as [start, end], or, if
    // not, in what format
}

@ptgott
Copy link
Collaborator Author

ptgott commented Jul 12, 2017

@NealHumphrey The most recent set of commits merges the dev branch (as of today) into this topic branch, then gets the date filter component to modify the displayed project dots. The next step will be to change the component per your suggestions.

@NealHumphrey
Copy link
Collaborator

@ptgott I put most of my suggested comments (re: removing tooltips etc.) into the issue #395 . This one describes the specific changes related to continuous filters but since you're using the same slider and approach for date picker I think we want to make all the same changes and then just swap out the input box for a datepicker box. Assuming you're going to do those next as you say, you should probably just claim issue #395 and do at all in one swoop.

Regarding the suggestion for the slider just moving year by year, I saw that noUiSlider has a 'steps' option - that might be the easiest way to cleanly implement this!

Maybe also implement steps for integer-type continuous sliders so they end up with clean integer numbers? Currently we're doing rounding on the front end and on the tooltip, disguising the fact that the user may have technically selected a decimal value, but actually storing the integer by forcing the user to select an integer via slider would also clean up the encoding of state in the url that we will be doing.

NealHumphrey added a commit that referenced this pull request Jul 12, 2017
…or merging the layer and filter functionality. These have been checked off in the ticket.

"Location" filtering still not implemented, but most everything else. This still leaves in a bit of tech debt in how we structured the code initially vs. how it would be ideally structured based on our current approach. But, it does a few must-have tech debt fixes and moves us in the right direction - I think enough for now with the exception of some filterview renaming as noted below.

@ptgott and/or @ostermanj can you do a review? Would definitely like to merge soon to avoid merge conflicts with future work since I touched a lot of places in the code.

- Combines all the config information for the data to be shown on the left hand side into one single 'dataChoices' variable.
- Renames the 'name' of the layers to be 'source', which is the equivalent field name that is used in the filterComponents. Ultimately I would like to again rename 'source' to be the more intuitive 'field_id' but am waiting to do that until @ptgott completes his changes in #385 to avoid merge conflicts. I'd also like to do some additional renaming of variables in filterView again to reduce tech debt by making variable names more accurately reflect their status.
- Comments out the creation of the layers panel - keeping this in the code for now until we do a quick couple user tests to make sure we don't need to revert to the two-controls method (seemed unlikely based on planning conversations)
- Creates and tears down the chloropleth legend, which has been moved onto the map itself. Chloropleth legend could be reformatted if we want, but at least it's visible now.
@ptgott
Copy link
Collaborator Author

ptgott commented Jul 13, 2017

@NealHumphrey I've merged the most up-to-date dev branch into this PR and tweaked the Date slider to select only years.

Since the larger tweaks left to do to the Date filters also apply to other continuous filter components, shall we merge this (if it's working on your end) and devote the work in #395 to additional PRs?

@NealHumphrey
Copy link
Collaborator

@ptgott I agree on pushing the other stuff into future PRs and merging this post testing. Unfortunately can't do it right now though, I'll circle back to it tonight. I'm putting one note of a change to make that I saw in a quick skim of the code though.


},
dateFilterControl: function(component){
filterView.filterControl.call(this, component);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only comment for now on a quick skim - you should switch out the appending of title text that is done step by step below for a call to the filterView.setupFilter(c); function that is used in the other controls. I refactored it this way so that we wouldn't duplicate the setup of title with click functions to open/close accordion etc.. It requires a slight change to how you actually append the slider itself, since setupFilter returns the "content" div

@ptgott ptgott closed this Jul 20, 2017
@ptgott ptgott deleted the 317-add-date-filter branch July 20, 2017 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants