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!: Return originally passed data as select events instead of mutated ones. #2151

Merged
merged 2 commits into from Oct 5, 2023

Conversation

mturoci
Copy link
Collaborator

@mturoci mturoci commented Oct 4, 2023

@marek-mihok - can you make a code review and a thorough QA to check if everything works as expected?

Breaking change alert

  • The returned event data is now the same as passed in. Prior to this PR, the data could have been mutated eg when scale='time' to timestamps.
  • Slight return format change: The events are returned in a flat array instead of a nested ([[]]) array for line, area plots etc.

Copy link
Contributor

@marek-mihok marek-mihok left a comment

Choose a reason for hiding this comment

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

LGTM @mturoci!

But note that taking into account possible override of idx value (that user specifies idx as a field name) does not make sense since the idx is overiden in refactorData on line 591 anyway:

ds.forEach((d, idx) => d.idx = idx)

You can try replacing value with idx in the plot_events_disabled.py example and see for yourself. Data are plotted with 0,1,2,3,4... indexes regardless of what indexes are specified.

image

It's worth considering fixing.

@mturoci
Copy link
Collaborator Author

mturoci commented Oct 5, 2023

Good point. Not related to the events though so will fix in another commit.

@mturoci mturoci merged commit bb9c1cf into main Oct 5, 2023
2 checks passed
@mturoci mturoci deleted the fix/plot-events branch October 5, 2023 13:55
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