Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Enhancement: add property Graph.prependData to support Plotly.prependTraces #850

Merged
merged 3 commits into from
Sep 4, 2020

Conversation

sleighsoft
Copy link
Contributor

Very similar to #461 but adds prependData capabilities which are already supported by Plotly.prependTraces.

@sleighsoft
Copy link
Contributor Author

Would love to see this feature added in the near future!

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

@sleighsoft I agree, this is a nice feature! I wonder if we can DRY the code a bit though?

  • I don't think EMPTY_PREPEND_DATA is necessary - just reuse EMPTY_EXTEND_DATA, maybe call it EMPTY_DATA - this array is never modified.
  • clearPrependData and clearExtendData look like they can be merged into perhaps clearState(key='prependData'|'extendData')
  • likewise prepend and extend are essentially the same function, just with prependData swapped in for extendData and prependTraces swapped in for extendTraces. So it could be generalized to something like:
    merge(props, 'prependData'|'extendData', 'prependTraces'|'extendTraces')
    or even
    merge(props, 'prepend'|'extend')

(don't DRY the tests though, those are good to keep distinct. Thanks for including the new test!)

@Marc-Andre-Rivet I don't imagine you'll be too excited about extendData getting a friend, given the headaches it creates, but do you have any other thoughts about this PR?

@alexcjohnson
Copy link
Collaborator

Also we'll need a changelog entry

@sleighsoft
Copy link
Contributor Author

sleighsoft commented Aug 27, 2020

Reworked it based on your suggestions. Let me know what you think :)
Will at the changelog later!

? {
extendData: EMPTY_EXTEND_DATA,
data: EMPTY_DATA,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
data: EMPTY_DATA,
[dataKey]: EMPTY_DATA,

Not sure how that would cause the prepend tests to fail while the extend tests still succeed... there may be something else funny going on. Aside from the fact that it broke the tests, yes this refactoring is exactly what I had in mind 😅

Might be easier to debug those locally with pytest -k test_graph_prepend_trace or, if you want to play with the test app before the automation commands run, pytest -k test_graph_prepend_trace --pause

(also the lint test failed, but that can be fixed with with npm run format)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not super familiar with JavaScript yet. What is that [..] syntax you are proposing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a guide on how to get the whole Chrome thing working for tests? I am on Windows with WSL and have not got it working yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When running the prepend and extend examples isolated on my machine (I bascially extracted the test code into a separate dash app .py file) then it works for both prepend and extend. Weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally got selenium working after upgrading to WSL 2. Will look into this in the coming days.

Copy link
Contributor Author

@sleighsoft sleighsoft Sep 3, 2020

Choose a reason for hiding this comment

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

Oh it took me so long to understand your suggestion... Total Javascript/React rooky here. Well, I learned something today.

@sleighsoft
Copy link
Contributor Author

sleighsoft commented Sep 3, 2020

Failing test-legacy ... I did not touch those.

For the other tests, running pytest -k test_graph_prepend_trace on my machine works

@sleighsoft
Copy link
Contributor Author

Added Changelog entry.

@alexcjohnson
Copy link
Collaborator

@sleighsoft the failing legacy tests have to do with a behavior change in the latest dash version, and they've been fixed in the dev branch. Can you merge dev back into this branch (and fix the changelog conflict)? That should fix these, then we can figure out what's going on with the new tests.

@alexcjohnson
Copy link
Collaborator

(The changelog conflict is normal, we just released a new dash & dcc version today)

@sleighsoft
Copy link
Contributor Author

I hope this is it now :)

@sleighsoft
Copy link
Contributor Author

Legacy still fails

@Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet I don't imagine you'll be too excited about extendData getting a friend, given the headaches it creates, but do you have any other thoughts about this PR?

@alexcjohnson Haha. Actually I like the unified solution + the extra test. Supporting both vs, one shouldn't make much of a difference 😄 My only concern atm is the https:// --> http:// change in the package-lock.json file. Seems that could just be 🔪 out.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 After a little test tweaking (unrelated to your code, these were just old tests that needed some modernizing to be more robust) and undoing the package-lock.json changes you had (which also happen for me locally, still don't get what's going on there) I think we're ready. Thanks for all your work @sleighsoft !

@alexcjohnson alexcjohnson merged commit 8a36e62 into plotly:dev Sep 4, 2020
@sleighsoft
Copy link
Contributor Author

Thank you for your help @alexcjohnson !

@Marc-Andre-Rivet Marc-Andre-Rivet added this to the OSS milestone Sep 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants