Skip to content

Ohlc trace type #264

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

Merged
merged 7 commits into from
Jan 23, 2018
Merged

Ohlc trace type #264

merged 7 commits into from
Jan 23, 2018

Conversation

VeraZab
Copy link
Contributor

@VeraZab VeraZab commented Jan 22, 2018

@VeraZab
Copy link
Contributor Author

VeraZab commented Jan 23, 2018

ok @nicolaskruchten would you like to give this a review?
hopefully I left enough notes about why certain things were necessary and the plotly.js specificities for financial charts

if you have a moment to take a look @bpostlethwaite I wouldn't refuse either :)

Copy link
Contributor

@nicolaskruchten nicolaskruchten left a comment

Choose a reason for hiding this comment

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

💃 seems reasonable to me. Not 100% sure I grok the implications of fullInput but it looks OK.

/>
</Section>

<Section name={_('Increasing Trace Styles')}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "Decreasing" instead of "Increasing" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, yes, 🙈

@@ -1,6 +1,6 @@
import React, {Component} from 'react';
import ReactDOM from 'react-dom';
import plotly from 'plotly.js/dist/plotly-basic';
import plotly from 'plotly.js/dist/plotly';
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm this might cause an issue with webpack when uglifying... The test I'm running now is almost at 10 minutes and it's still not done :(

Copy link
Contributor

Choose a reason for hiding this comment

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

-basic took 153s to complete with webpack -p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but this is what I need to get all my other chart types..
if I don't have this, I only get the basic 5 chart types, I think it's ok that we add a fuller version into the dev env

Copy link
Contributor

Choose a reason for hiding this comment

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

for the dev env ok but this won't work for something like the simple app: we'll need to load plotly.js the proper way i.e. not from dist :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I see : ) ok, well, I'll merge this for now

@VeraZab
Copy link
Contributor Author

VeraZab commented Jan 23, 2018

I'll link this issue here for reference, about the difference of _fullInput vs _fullData: plotly/plotly.js#2281

But, I know that for composed traced types, like financial charts, the true attributes of the trace are in _fullInput. However, _fullInput doesn't seem to contain everything that _fullData contains.

Well, we should be good for now I think :)

@VeraZab VeraZab merged commit 5bfb61c into master Jan 23, 2018
@VeraZab VeraZab deleted the ohlc branch January 23, 2018 20:23
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.

2 participants