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

395 Control date filters via text input #411

Merged
merged 17 commits into from
Jul 31, 2017
Merged

395 Control date filters via text input #411

merged 17 commits into from
Jul 31, 2017

Conversation

ptgott
Copy link
Collaborator

@ptgott ptgott commented Jul 14, 2017

Addresses #395.

  1. I've included a constructor for an input element that changes the state of a filter, something we should be able to use between date filters and continuous filters.
  2. I've opted for input boxes over a calendar-style date picker because it is faster and more direct, assuming that the user doesn't need to know the day of the week a date falls on.
  3. The filter input constructor allows the user to bind the input to a callback, which fires when 'tab' or 'enter' are pressed, or when the input element loses focus.
  4. Setting the state of the date filter values is bound to the input element, not the slider.
  5. Changing the slider changes the input element's year and triggers its callback.
  6. Changing the input element changes the state of the slider without triggering its callback.

This subsumes #385

@ptgott
Copy link
Collaborator Author

ptgott commented Jul 14, 2017

Looking over #395 again, I've noticed that the last few commits neglect some implementation details. I hadn't given the issue a second look before making the commits, and can make the changes this weekend if you'd prefer.

  1. They add the includeNulls key to dataChoices, rather than the state object. I've done this out of convenience, because the date pickers also change properties of dataChoices, taking it as an object that reflects the state of the data between the initial load and its use by the UI. There's probably a better object to use for this. I can move includeNulls to the state object, as intended, but I'm not sure about where to put the re-assigned min and max of the dataChoices objects of type 'date'.
  2. There's no submit button. It'd be easy to add one.
  3. The text by the switch is slightly different and the switch handle is two-dimensional.

Edit I've also forgot to set the 'cursor' CSS of the toggle switch to 'pointer'.

@NealHumphrey
Copy link
Collaborator

@ptgott Sorry it took me a couple days to get to this.

Looks like good progress and getting close. I agree, though, that we should go ahead and make the changes you note.

You should not use the dataChoices object for any user interactions - we should put everything that can be changed by the user onto the State object. a) this makes it cleaner to separate configuration from user changes and b) we'll definitely need this for #387 - URL State Encoding.

The 'min' and 'max' values in dataChoices were originally used to determine the outer bounds of the sliders. As it appears you've done for the date slider this should actually be set by checking the data itself to find the min and max values. Note that the continuous number sliders have not yet implemented this functionality and still use dataChoices.max and dataChoices.min for their slider configuration.

The best place to put this is on the state object with they keyword filterValues.my_source_name where my_source_name is the value stored in the 'source' field of dataChoices - the unique identifier of the data being filtered on. In the ticket I suggested passing a triplet of data instead of a pair with setState('filterValues.portfolio', [minDateSelected, maxDateSelected,includeNullsBoolean]).

Currently the filter pillboxes are not working in this branch - clearing the filter doesn't clear the pillbox, and clicking the x on the pillbox doesn't reset the filter. That fix may be part of the setState issue.

Final couple issues you note should be easy -submit button, null value text, and pointer cursor in css.

Thanks Paul!

Copy link
Collaborator

@NealHumphrey NealHumphrey left a comment

Choose a reason for hiding this comment

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

See my comment. We should make the 'dataChoices' object immutable, and store everything in the State object so that we can recreate it from the URL when needed and so triggering pillboxes and such works correctly.

…min and max values for continuous filter inputs from dataCollection.filterData where available, and from dataChoices as a provisional measure in case this is not available. The date filter control stores the min and max values of newly formatted dates as local variables, not in dataChoices.
@ptgott
Copy link
Collaborator Author

ptgott commented Jul 19, 2017

@NealHumphrey
The latest commit has addressed the issues re: storing the nullsShown state and touching dataChoices. I'll get to the cosmetic stuff/submit button later in the week.

I've opted to use a separate key within the state object to store the values of nullsShown, rather than make nullsShown a third element within a given state array, as there are a number of moments throughout the code that assume that an empty filterValues state array indicates a cleared filter.

@NealHumphrey
Copy link
Collaborator

Great, thanks @ptgott! I'll wait to get the cosmetic stuff before doing a final review/merge, let me know when that's ready.

…' and 'max' filter control text inputs into the same constructor. Format the date and continuous filter control components.
@ptgott
Copy link
Collaborator Author

ptgott commented Jul 20, 2017

@NealHumphrey The newly formatted date/continuous filter controls should be ready now!

@NealHumphrey
Copy link
Collaborator

NealHumphrey commented Jul 24, 2017

@ptgott this is great! It's almost ready, there's just a couple things I'd still like to adjust before it gets merged in.

  • The biggest change - the choice to include/exclude nulls should actually be part of the filtering choices. Currently, if I select a filter such as Subsidy Start Date with the slider, and then I click the toggle so that unknown values are excluded, I end up with some filtered buildings. Then, I click the 'clear' pillbox and that filter disappears - from the pillboxes there is no evidence I have an active filter. But, I still have gray buildings that do not match my filter, because my 'clear' pillbox does not clear the choice I made. This is confusing, and its not clear how to get back to the reset state.

    • Clearing the filter should reset the null value setting to its default state (included)
    • Toggling the null value filter to exclude nulls should trigger the creation of a pillbox to indicate which data choice is filtered via nulls
      The cleanest way to do this might be to use a third value in the filterValues state ([min,max,null]) - as you noted there are many places in the code that assume that an empty filterValues means no filters are selected, but this is actually the behavior we want. If you want to do it a different way and keep your separate null values state that's fine too.
  • The null values toggle currently isn't very usable. I probably shouldn't have specified a slider toggle because despite their ubiquity they have pretty bad UX performance - they're confusing. Should we just change this to a checkbox?

  • Change the css for text-input margin-left to be 20px (date picker submit button currently wraps when there is a scroll bar shown, while the 20px margin fixes it).

  • in dataChoices.js, remove all the 'min' and 'max' settings - since these are now calculated in the code based on the data, we don't need them in the configuration data so should clean it up by getting them out.

  • Merge dev into this branch and fix conflicts.

@NealHumphrey
Copy link
Collaborator

Also - do merge in the current dev branch to resolve your merge conflicts so it's ready to get into dev

ptgott added 2 commits July 27, 2017 21:17
…ndividually with pillboxes. Clear multiple 'nulls shown' filter values with the Clear All button.
@NealHumphrey
Copy link
Collaborator

@ptgott it looks like you've checked off all the checklist - is there anything left that you were planning to do on this?

All the changes look good. I did find a little bit of UI bug, though, related to the syncing of the UI and the actual filter state. I made a separate ticket for it. #442

Let me know if there's anything else you expect still needs changing. I think that I will probably merge this in as-is, and let you/someone pick up the issue #442 in a separate PR so that we can use this new ui in a few integration related tasks.

@NealHumphrey NealHumphrey merged commit 9fbbb90 into focusconsulting:dev Jul 31, 2017
@ptgott
Copy link
Collaborator Author

ptgott commented Jul 31, 2017

@NealHumphrey Thanks for taking a look at this!

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.

2 participants