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

Revise scatter register to allow custom partial bundles without scatter included by default #5535

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Mar 8, 2021

Follow up of #5527,
Registering scatter trace outside core via lib/scatter (which used to be unused) so that one could create custom partial bundles without scatter+cartesian overhead.

cc: #5395

@plotly/plotly_js

@archmoj archmoj added this to the NEXT milestone Mar 8, 2021
@alexcjohnson
Copy link
Collaborator

Pretty cool, doesn't seem like it was that hard to make this work 😎

We'll need to do something about the default trace type - the two options that occur to me are (1) use the first trace type registered, or (2) if scatter isn't registered and you give no (valid) type, throw an error.

In addition to the bundle tests verifying behavior without scatter present, we should test components (shapes, annotations, images) and make sure something reasonable happens when the cartesian framework is missing. Perhaps that can be a silent failure, though maybe better if we could log a warning of some sort.

@nicolaskruchten
Copy link
Contributor

Can we timebox this PR's remaining work to 1 hour please? This one is out of scope for the work in v2 at this point, and getting RC.1 out the door and fixing the bugs on the roadmap should take priority now :)

@archmoj
Copy link
Contributor Author

archmoj commented Mar 9, 2021

Pretty cool, doesn't seem like it was that hard to make this work

We'll need to do something about the default trace type - the two options that occur to me are (1) use the first trace type registered, or (2) if scatter isn't registered and you give no (valid) type, throw an error.

The second option is applied in 45d3764.

@archmoj archmoj force-pushed the allow-partial-bundles-without-scatter branch from 45d3764 to b0766a7 Compare March 9, 2021 01:19
Loggers.log('The default trace type ' + dfltTrace + ' is not registered.');

// in case the category starts with no return true otherwise false
return category.indexOf('no') === 0 ? true : false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it important to return true here for the no categories? In what situation would this occur?

@archmoj archmoj added status: reviewable bug something broken labels Jun 26, 2021
@gvwilson gvwilson self-assigned this Jun 10, 2024
@gvwilson
Copy link
Contributor

This pull request has been sitting for a while, so I would like to close it as part of our effort to tidy up our public repositories. I've assigned it to myself to keep track of it; I'll wait until 2024-06-17 for someone to say it's still relevant and they'll to take it on, and otherwise I will close it then. Thanks - @gvwilson

@gvwilson gvwilson removed their assignment Aug 2, 2024
@gvwilson gvwilson added feature something new and removed status: reviewable bug something broken labels Aug 8, 2024
@gvwilson gvwilson added the P2 needed for current cycle label Aug 9, 2024
@gvwilson
Copy link
Contributor

@archmoj this is tied to a ticket you closed in 2021 - can we please close this PR?

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

Successfully merging this pull request may close these issues.

4 participants