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

392 zone filters + continuous and nulls refactor #451

Merged
merged 14 commits into from
Aug 9, 2017

Conversation

NealHumphrey
Copy link
Collaborator

This pull request includes a variety of changes to the filterControl elements, primarily for continuous controls but also for date controls, and to the connected filter.js, router.js. Note that this PR also includes #448 which did some simple rearrangment of the continuous filter. Included:

  • Any continuous control can have its min/max values reset at any point.
  • Zone-based continuous filters have their values cleared and min/max values reset whenever the zone type is changed.
  • Rearranges how values of the UI components (checkbox, textbox, slider) are bound together. If adjusting one of the UI components, the new state is determined by what is currently in the other UI components. This way if the internal state somehow gets out of sync with the UI state, it will self correct whenever a user uses any of the UI for that filter (see bugs resolved below)
  • Stores all the UI components on the filterControl itself so they can be accessed by other code.
  • Removes the state of nullsShown.<source> in favor of using the 3rd value of the filterValues. state array (i.e. [min,max,nullValue]). The URL encoding code already added this to the state, which was duplicative, and we want the checkboxes to cause the same reactions in the UI as the sliders re: pillboxes etc. - @ptgott created the nullsShown state to separate the checkboxes from the code that executes filters, but that turned out to not be the desired behavior and maintaining two separate states was more complexity.

This also fixes a few bugs that existed in the current dev branch:

  • When loading a saved state, the checkbox for nulls could get unsynced from the values when includenulls was false, meaning the checkbox showed the opposite of what the state was. (found during dev, no issue created)
  • Similar bug when executing filter updates in a certain order as described in bullet 2 of Filter UI not reset properly in some circumstances #442

Additional bonus feature:

  • Changes how the setState() command logs to the console. It now prints a grouped log that includes a stack trace, so coders can see which file initiated the new state for easier debugging.

NealHumphrey and others added 11 commits August 7, 2017 12:24
…ns we can setState of our filterValues multiple times if needed and if the values haven't changed it won't be published. This should make it easier to resolve some of our sequencing problems for not running the filterData functions multiple redundant times.

- Adds a 'set' method to the continuous filter control, which centralizes where all the UI and state elements get set at once.
- Refactors the router to use this set method.
…m to be recalculated when changing zone types.

zone-level data sets now getting proper min/max levels on load.
Temporarily removes fraction_black for user testing to avoid race discussion - we will be adding all races in v2.
…nuous

392 zone filters part of continuous
…e proper min/max levels, and clears any activated filters
TODO:
the Nulls checkbox is now broken - throws an error.
@NealHumphrey
Copy link
Collaborator Author

Due to the fact that we're having a user test tonight at 8pm, I've merged this into the live website so that the zone-based filters will operate correctly for this test. This PR is into dev, so if there are any changes needed to the PR I'll work out the Git merging needed to get everything up to date when it's all resolved.

@ptgott this PR is in conflict with your PR #444 and also undues some of your code on the nulls from a couple weeks ago. I'm sorry for that, it's very soon for a refactor and I hope it's not too much of a dissapointment! There were a few conflicts and bugs in the existing system and it was easier for me to rearrange some of it rather than patch.

@ostermanj
Copy link
Collaborator

Changing zone types doesn't seem to be clearing the zone-based filters the way we expect. @NealHumphrey

@ostermanj
Copy link
Collaborator

@NealHumphrey : as discussed, I'll approve and merge this PR and write up the outstanding bugs as new issues so that other PRs are on top of this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants