Skip to content

Bug fix: indexFromSetName on invisible sets#771

Merged
danvk merged 3 commits into
danvk:masterfrom
squaregoldfish:master
Aug 26, 2016
Merged

Bug fix: indexFromSetName on invisible sets#771
danvk merged 3 commits into
danvk:masterfrom
squaregoldfish:master

Conversation

@squaregoldfish
Copy link
Copy Markdown
Contributor

@squaregoldfish squaregoldfish commented Aug 25, 2016

Calling indexFromSetName on the last set would fail if its visibility was set to false, because the set's label would not be added to the internal list of sets (stored in the setIndexByName_ variable).

This pull request fixes the issue, and adds a test to check that it works.

The test added to tests/data_api.js provides an example that currently fails, and now passes with this fix.

If the last set(s) have their visibility set to false, their names
would not be added to `setIndexByName_`, so `indexFromSetName` would
not be able to find them. This fixes that problem.
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.02%) to 89.563% when pulling ed1f514 on squaregoldfish:master into 546df89 on danvk:master.

Comment thread auto_tests/tests/data_api.js Outdated
// In 1.1.1, if you request the last set and it's invisible, the method returns undefined.
it('testIndexFromSetNameOnInvisibleSet', function() {

var localOpts = opts;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This doesn't actually copy anything. If you want to do that there's a method in utils.

@danvk
Copy link
Copy Markdown
Owner

danvk commented Aug 25, 2016

Other than my one comment, this looks great. Thanks for the fix!

Will this address #434 and #637?

@squaregoldfish
Copy link
Copy Markdown
Contributor Author

squaregoldfish commented Aug 25, 2016

Yes, I think it addresses both those issues.

I'll take a look at the test and update it tomorrow.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.02%) to 89.563% when pulling 5579bab on squaregoldfish:master into 546df89 on danvk:master.

@danvk danvk merged commit 051a854 into danvk:master Aug 26, 2016
@danvk
Copy link
Copy Markdown
Owner

danvk commented Aug 26, 2016

Squashed & merged. Thanks!

@squaregoldfish
Copy link
Copy Markdown
Contributor Author

NP. Don't forget about #434 and #637 :)

@danvk
Copy link
Copy Markdown
Owner

danvk commented Aug 26, 2016

Thanks for the reminder! Closed them out.

PierrickKoch pushed a commit to PierrickKoch/dygraphs that referenced this pull request Sep 15, 2016
* indexFromSetName works with trailing invisible sets

If the last set(s) have their visibility set to false, their names
would not be added to `setIndexByName_`, so `indexFromSetName` would
not be able to find them. This fixes that problem.

* Corrected test.

* Properly clone graph options
PierrickKoch pushed a commit to PierrickKoch/dygraphs that referenced this pull request Sep 22, 2016
* indexFromSetName works with trailing invisible sets

If the last set(s) have their visibility set to false, their names
would not be added to `setIndexByName_`, so `indexFromSetName` would
not be able to find them. This fixes that problem.

* Corrected test.

* Properly clone graph options
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.

3 participants