Offline mode is now default for plotly(). #357
Conversation
|
Tested it. Note: When online mode is used, this returns the url to chart-studio |
|
@harshpurwar can you please review as well? |
There was a problem hiding this comment.
First things first, this doesn't work as one would expect it to, see the attached screenshot (not truly offline!). Now, I know I see this because I haven't set up plotly with API Key from Chart Studio. But that is the point, we shouldn't be needing the API key for offline usage, right @jackparmer?
The solution for this is already in the master branch. So, if plotlynofig.m was taken from the current master, this would have worked.
Now the second thing I would like to comment on is the fact that we don't want to have two similar classes plotlyfig and plotlynofig, the differences between the two aren't too many (see the attached file: diff_fig_noFig.txt). Is there a reason why we couldn't do this by checking the number & type of input arguments and then calling the right functions all within the same class? I don't think it is a good idea to have two similar classes plotlyfig and plotlynofig, it just calls in for a lot more future work!
|
I am a physicist, and under pressure, I wanted and figured out something
that works. What is it you propose that works?
…On Wed, Aug 25, 2021, 22:19 Purwar ***@***.***> wrote:
***@***.**** requested changes on this pull request.
First things first, this doesn't work as one would expect it to, see the
attached screenshot (not truly offline!). Now, I know I see this because I
haven't set up plotly with API Key from Chart Studio. But that is the
point, we shouldn't be needing the API key for offline usage, right
@jackparmer <https://github.com/jackparmer>?
The solution for this is already in the master branch. So, if
plotlynofig.m was taken from the current master, this would have worked.
Now the second thing I would like to comment on is the fact that we don't
want to have two similar classes plotlyfig and plotlynofig, the
differences between the two aren't too many (see the attached file:
diff_fig_noFig.txt
<https://github.com/plotly/plotly_matlab/files/7049913/diff_fig_noFig.txt>).
Is there a reason why we couldn't do this by checking the number & type of
input arguments and then calling the right functions all within the same
class? I don't think it is a good idea to have two similar classes
plotlyfig and plotlynofig, it just calls in for a lot more future work!
[image: image]
<https://user-images.githubusercontent.com/38316470/130863737-c6dba946-c5ce-4481-b7cf-8665ac332920.png>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#357 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEQOHL5JLJUZ3HX6LXYZLEDT6VM4PANCNFSM5CXPYWJQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
@harshpurwar I have tried what you suggested and it is not working. I know for sure that what I did is working and @gilbertogalvis confirmed it! Maybe you are not having the correct files. By the way, there was no plotlynofig before, it is a new file that I added to try to mimic what plotlyfig is doing but this time when passed only data and NOT a figure. I understand your worries about having two similar classes but I guess it is also good to know that one is from a figure and the other is not. I think you are confusing between "fig2plotly" and "plotly". fig2plotly takes in a matlab figure, while plotly takes in data with some layout. |
|
correct - an API key should never be required for offline usage at any point |
|
I also agree that we don't want two classes that are essentially the same. We need to keep the code DRY (don't repeat yourself) so that it is maintainable |
|
please merge this PR for now @jolinos so that your plotly() documentation work is unblocked. @harshpurwar can then fix some of these remaining issues in a follow up PR later. For now the important task for you @jolinos is the plotly() documentation pages. |
Offline mode is now default for plotly as in the example below.