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

adding support for defining groups in data.transforms.groups using ty… #4410

Conversation

danieljblinick
Copy link
Contributor

the idea here is just to allow groups to be defined in data.transforms using typed arrays as well as regular arrays, to save on space

@danieljblinick
Copy link
Contributor Author

@etpinard not sure how to assign you as an official reviewer (first time doing this kinda thing)

@etpinard
Copy link
Contributor

etpinard commented Dec 6, 2019

Thanks very much for the PR @danieljblinick !

Would you be interested in adding a test to lock down the behavior? Adding one it block in transform_groupby_test with a Plotly.plot and a few gd._fullData assertions should be enough. Let me know if you need help.

@danieljblinick
Copy link
Contributor Author

@etpinard added a test, lemme know if thats what you had in mind.
some tests are failing, but i think theyre unrelated to my changes, and noticed they're tagged flaky

@@ -318,6 +332,22 @@ describe('groupby', function() {
.catch(failTest)
.then(done);
});

it ('Plotly.plot should group points properly using typed array', function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should be

it('Plotly.plot should group points properly using typed array', function(done) {

to make npm run lint pass.

})
.catch(failTest)
.then(done);
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should be

});

to make npm run lint pass.

@etpinard
Copy link
Contributor

etpinard commented Dec 9, 2019

@danieljblinick your test looks good. Nice job!

You'll need to push another commit to make the lint test (npm run lint) pass though. You were correct, the other failures were from flaky tests, after restarting their test run, they are now green. Thanks again!

@danieljblinick
Copy link
Contributor Author

@etpinard seems like all is passing! lemme know if there is anything else :)
thanks for all ur help

@etpinard etpinard added this to the v1.52.0 milestone Dec 9, 2019
@etpinard
Copy link
Contributor

etpinard commented Dec 9, 2019

Thank you very much @danieljblinick !!

This will be part of our upcoming v1.52.0 release.

@danieljblinick
Copy link
Contributor Author

@etpinard is there any way you can send me a tagged release with this feature? i would really appreciate it if its possible, cause we kind of need the feature. thanks!

@alexcjohnson
Copy link
Collaborator

@danieljblinick I believe we're planning the next release in early January. Can you just use the built files from this PR? You can grab whichever flavor you need from here:
https://circleci.com/gh/plotly/plotly.js/61067#artifacts/containers/0
ie the full minified bundle is:
https://61067-45646037-gh.circle-artifacts.com/0/dist/plotly.min.js

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

Successfully merging this pull request may close these issues.

3 participants