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

Fix/recursive corner case #170

Merged
merged 2 commits into from
Oct 27, 2020
Merged

Fix/recursive corner case #170

merged 2 commits into from
Oct 27, 2020

Conversation

breezykermo
Copy link
Member

This PR fixes two corner cases introduced by the coloring set algorithm that can cause the state of active filters and color to get way out of whack. (There is undefined and hard-to-interpret behavior whenever an incorrect action is dispatched for toggling filters, or for updating the coloring sets, as these two aspects of the state are tightly coupled.)

  1. The getFilterSiblings function was always returning an empty array for meta filters (implicit filters that exist as parents in filter_paths, but not as associations in the sheet). This is because the function was only checking the ids of assocations, and not recursing/reducing up the filter_path array to find implicit filters. getFilterSiblings now delegates to getMetaFilterSiblings when the filter key passed is not an association, and returns the appropriate siblings. This was causing havoc in terms of appropriately toggling off grandparents when parent sets had been toggled off after toggling on a grandparent.

  2. Similarly, the control flow that toggles off a paren when its last child is toggled off was not seeking further up the family tree, and appropriately toggling off grandparents as well if necessary. This also caused coloring havoc- now fixed.

@breezykermo
Copy link
Member Author

@ebefarooqui feel free to merge once you've reviewed. @zacoppotamus and I agreed that we can dramatically simplify the logic in components to make this more maintainable, most probably by 'serializing' the filter_path hierarchy to a tree-like structure when the domain is first loaded and keeping this in the Redux store. Then we can use this as the canonical representation and recurse down it to find filters: rather than having to always deal with the two variants of filters, association-filters and meta-filters, at every point in the control flow.

@ebefarooqui ebefarooqui merged commit 68814d5 into develop Oct 27, 2020
@breezykermo breezykermo deleted the fix/recursive-corner-case branch October 28, 2020 07:40
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