The Wayback Machine - https://web.archive.org/web/20220809055741/https://github.com/plotly/plotly.js/pull/5535
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

Jump to bottom

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
Open

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

wants to merge 6 commits into 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
Contributor

@alexcjohnson alexcjohnson commented Mar 8, 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.

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
Member

@nicolaskruchten nicolaskruchten commented Mar 8, 2021

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 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 Mar 9, 2021
alexcjohnson
alexcjohnson reviewed Mar 9, 2021
View changes
src/registry.js
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
Contributor

@alexcjohnson alexcjohnson Mar 9, 2021

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 removed this from the NEXT milestone Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcjohnson alexcjohnson

Assignees
No one assigned
Labels
status: reviewable type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@archmoj @alexcjohnson @nicolaskruchten