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

Tests and tweaks to the select menu widget #1019

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

madebydna
Copy link
Contributor

There are now tests for the no-values, one-value, and multiple-values case in multiple select mode.

Also improved / refactored some implementation details. @gordonwoodhull, as per your suggestions I changed the implementation of _chart.onChange to always call replaceFilter with an array containing the array of filters reflecting the current selections. This works for both the single and multiple select mode.

Additionally the empty option (prompt option) is always filtered out, so when the prompt option is the only one selected, then the array will be empty, i.e. the select is re-set.

The demo under web/examples/select.html was very helpful :) I was wondering about was the behavior in multiple mode where the prompt option can be selected along with other options. Would the correct / expected behavior here be to override and deselect everything when that option is selected?

screen shot 2015-09-28 at 8 39 58 pm

In any case, the actual onChange callback's only responsibility is now to forward the selected options from the DOM element (and sort out browser differences). This also seems to make the _chart.onChange method more testable since one can easily call the method with different combinations of selections. I've left in the option to send null into _chart.onChange, which is mostly a convenience in tests, but maybe confusing. Let me know if I should remove it.

 in multiple select mode.
 Also improved / refactored some implementation details.
@gordonwoodhull
Copy link
Contributor

Thanks @madebydna! I'm still not quite free of other deadlines but this sounds great.

As for the expected behavior when the prompt is selected, it feels natural to me that the default is all-selected and is easily chosen with the prompt. Even though it may seem inconsistent or illogical, it's the most common choice and IMO is similar to the way the other ordinal selections work (pie, row, and ordinal bar charts).

I am considering also adding a "reverse" or "invert" option, since excluding some options is also a common thing to do. (Right now I'm using the widget to select certain node types to filter out in a network diagram in order to make it readable.)

@gordonwoodhull
Copy link
Contributor

We should also test that the selected options are the same as the filters for the multi case. I think I found a bug where when the filters are set it only adds items and doesn't remove any.

gordonwoodhull added a commit that referenced this pull request Oct 2, 2015
here is another thing that should be tested (re: #1019)
@gordonwoodhull gordonwoodhull modified the milestone: v2.1 Oct 6, 2015
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.

None yet

2 participants