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.

Sign up for GitHub

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

[WIP] Browsable time slider #31

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[WIP] Browsable time slider #31

wants to merge 4 commits into from

Conversation

IshaGupta18
Copy link
Collaborator

@jywarren @Souravirus here's the code for #22. Lets work to resolve this bug or find a workaround for this

@IshaGupta18 IshaGupta18 mentioned this pull request May 28, 2019
Open
@Souravirus
Copy link
Member

Hi, this caught my attention that actualType should be replaced with pie, line or bar. What do you think??

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented May 28, 2019 via email

@IgorWilbert
Copy link
Member

Hi @IshaGupta18 ! I had a feeling similar to @Souravirus . Maybe we could use graphType or something similar instead of actualType. What do you think?

@Souravirus
Copy link
Member

Yeah understood @IshaGupta18

@IshaGupta18
Copy link
Collaborator Author

So anyone has any ideas?

@jywarren
Copy link
Member

jywarren commented May 29, 2019 via email

@IshaGupta18
Copy link
Collaborator Author

Alright, the name of the variable is changed now. What do you all think might be causing the error?

@jywarren
Copy link
Member

I think it may be necessary to wrap your code (esp the line causing the error) in a function which can be run after all the prerequisites are loaded. I think it may be a load-time issue. You can wrap your code in:

$( document ).ready(function onReady() {
  // your code here; it'll run after jQuery is loaded!
});

If that doesn't work, a more basic way to do this is:

(function onReady() {
  // this code will be delayed in execution until the page is completely loaded
})()

Worth a try!

@IshaGupta18
Copy link
Collaborator Author

I tried putting up the entire code, since its all co-dependent, but the error still persists, I feel it was something to with the initial variable, but I am not able to figure it out, since the code is very similar to this working fiddle here http://jsfiddle.net/schme16/xfyvvup8/. What do you think?

@IshaGupta18
Copy link
Collaborator Author

Does anyone have other ideas that could be tried on? @Souravirus. @sagarpreet-chadha @rexagod, could you have a glance here?

@IshaGupta18
Copy link
Collaborator Author

@IgorWilbert @geekychasser any suggestions?

@IgorWilbert
Copy link
Member

IgorWilbert commented Jun 2, 2019 •
edited
Loading

@IshaGupta18 sorry, could you please recap? What is the error message you are getting now? I saw that on #22, you solved the carroussel problem.

@IshaGupta18
Copy link
Collaborator Author

So basically, right now I am facing this error
image
Which is coming because of the range-slider CDN.

@IshaGupta18
Copy link
Collaborator Author

I tried putting up the entire code, since its all co-dependent, but the error still persists, I feel it was something to with the initial variable, but I am not able to figure it out, since the code is very similar to this working fiddle here http://jsfiddle.net/schme16/xfyvvup8/. What do you think?

Refer to this comment as well!

@IgorWilbert
Copy link
Member

@IshaGupta18 I hope this does not sound obvious. Your CDN is trying to use splice of an undefined value, similar to this issue You should check the code in this link https://gitcdn.link/cdn/schme16/Chart.js-RangeSlider/677a7eb6c295772606d300697eaf552245e7f171/dist/RangeSlider-all.min.js , especially this line var arrayEvents = ['push', 'pop', 'shift', 'splice', 'unshift']. I could not find the problem in the fiddle you mentioned, is it a different issue?

@IshaGupta18
Copy link
Collaborator Author

@IgorWilbert , so the fiddle works fine, and my implementation is very similar to the fiddle, that is why, I am not able to understand, why this one is creating a problem. I think it was to do something with the initial variable, which is being undefined for splicing, or the entire thing could be wrong, the error occurs at the last line of the range slider file.

@IgorWilbert
Copy link
Member

@IshaGupta18 I see. I guess it is unlikely that there is a problem with the initial variable, unless if only a given range is allowed (you could test changing its value to [3, 10] for example). Are you able to print to the console the props of RangeSliderChart ? The values for chartData, chartOpts, chartType, chartCTX, class and initial are all valid? If yes, then it might be that the browser needs the graph before it is rendered. In this case, you could try assigning new RangeSliderChart to a variable and setting a timeout when calling the variable: Window.setTimeout(myVariable(), 3000);

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jun 2, 2019 via email

@IshaGupta18
Copy link
Collaborator Author

Okay so we have made some progress here, the bug is removed now, however, there is a new problem, which seems fixable but I can't seem to recall how:
I think we are very close now, could someone tell why this might be happening, I even tried delaying the output, but to no use
@jywarren @Souravirus @IgorWilbert
error

@Souravirus
Copy link
Member

Hi @IshaGupta18, in this line, just call the getContext function as is told in this answer: https://stackoverflow.com/a/48895731/7806855

simple-data-grapher/src/parsing.js

Line 175 in de6e403

var ctx = canv.getContext('2d');

See if this works.

@jywarren
Copy link
Member

jywarren commented Jun 4, 2019

Hmm, can you describe the problem? I think what I'm seeing is the graph getting generated over and over.

@IshaGupta18
Copy link
Collaborator Author

@Souravirus I think this didn't work out.
@jywarren I think I have encountered this problem before but I think I don't know how I fixed it. Even I think the graph is loading over and over again. Any fix?

@IshaGupta18
Copy link
Collaborator Author

Okay, so before I could have started debugging this yet again, I found that the cdn link is broken, which was working fine till yesterday, but all of a sudden it has stopped working. @jywarren @Souravirus does anyone of you have an alternative for the slider thing? Thanks a ton!

@Souravirus
Copy link
Member

Checking if I can get something @IshaGupta18.

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jun 7, 2019 via email

@Souravirus
Copy link
Member

Hi @IshaGupta18, see if it can be downloaded from yarn because it is available in npm also, here is the link for npm: https://www.jsdelivr.com/package/npm/chart.js-rangeslider

@IshaGupta18
Copy link
Collaborator Author

It doesn't work
image
I think there is some recent bug in their file itself

@IshaGupta18
Copy link
Collaborator Author

Any other alternatives for the slider?

@Souravirus
Copy link
Member

See if this rangeslider will work: https://rangeslider.js.org/

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jun 7, 2019 via email

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jun 7, 2019 via email

@Souravirus
Copy link
Member

Yeah fine for me if it serves the purpose.

This was referenced Jun 8, 2019
Closed
Open
@jywarren
Copy link
Member

Hi @IshaGupta18 I think what was suggested was that we include https://www.jsdelivr.com/package/npm/chart.js-rangeslider via NPM as a dependency in our package.json and then use it locally. Can you run npm install chart.js-rangeslider and reference it locally?

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jun 11, 2019 via email

@jywarren
Copy link
Member

jywarren commented Jun 11, 2019 via email

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jun 12, 2019 via email

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jun 12, 2019 via email

@IgorWilbert
Copy link
Member

Hi @IshaGupta18 ! Are you still facing stability issues with npm? If yes, what are the error messages you are getting?

@IshaGupta18
Copy link
Collaborator Author

@IgorWilbert I am yet to try the suggested approach. Once I do, I will bet back to you. Thanks a lot!

@IshaGupta18
Copy link
Collaborator Author

Okay so https://www.jsdelivr.com/package/npm/chart.js-rangeslider address doesn't contain the npm anymore, and here's the link to the issue: schme16/Chart.js-RangeSlider#17 which I am facing, however, it didn't work with this file as well, the test example they have given in their repo shows the same error. I am still gonna give a shot to the npm method somehow, but I am not too sure of its working.

@IshaGupta18
Copy link
Collaborator Author

So I included it through npm, however we are still supposed to include the script file as, so I tried that, and I am facing the same error:
image

I honestly feel that we should look for an alternative now, as this one is very unstable ie. it is not working within its own example (one in the repo and one in a fiddle) and it's not very regularly maintained. If we decide to shift to plotly.js, we won't be needing a slider at all, as zooming and panning is built in. What do you think @jywarren @IgorWilbert ?

@IgorWilbert
Copy link
Member

IgorWilbert commented Jun 16, 2019 •
edited
Loading

@IshaGupta18 have you make significant work (I mean, that it would take a long time to redo) based on chart.js? I believe what @jywarren meant in #34 was to use both plotly and chart.js by passing an identifier to a function. However, in my opinion, using both libraries can make the code a little harder to understand and maintain, so I would avoid doing that.

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jun 16, 2019 via email

@IgorWilbert
Copy link
Member

@IshaGupta18 yay, now we have plotly! Do you already know how we can continue working on the slider?

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jun 30, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@IshaGupta18 @Souravirus @IgorWilbert @jywarren