jamesleesaunders / d3-x3d

3D Data Driven Charting Library with D3 and X3D
https://jamesleesaunders.github.io/d3-x3d/
GNU General Public License v2.0
108 stars 22 forks source link

173 create base add to component #180

Closed mo-pat closed 3 years ago

mo-pat commented 3 years ago
mo-pat commented 3 years ago

173

jamesleesaunders commented 3 years ago

Hi @mo-jinbuu Thank you so much for contributing. This is a most excellent tidy-up.

Although the unit tests all passed there is possible still something amiss. If you look at the examples in the browser: /examples/X3DOM/chart/... there is a console error:

d3-x3d.js:5550 Uncaught TypeError: createBase is not a function
    at my (d3-x3d.js:5550)
    at Pt.call (d3.v5.min.js:2)
    at refreshChart (AreaChartMultiSeries.html:70)
    at AreaChartMultiSeries.html:82

If you could take a look at this what would be great, let me know if you get stuck and I could take a look also.

All the best. Jim

jamesleesaunders commented 3 years ago

Oh and I probably should have said, the failure above is with the dist files built (npm test till build the dist files).

mo-pat commented 3 years ago

Ah I see, alright I'll take a look at it. I'm just not sure how to go about running the program locally but I'll look into it. I'll message you if I need any help with that.

mo-pat commented 3 years ago

Oh nevermind, I didn't realize they were example html files available that I could use. This should help

jamesleesaunders commented 3 years ago

Do let me know if you need me to help test, if you get stuck give me a shout and I’ll see if I can work it out. Thanks again!

mo-pat commented 3 years ago

Hey sorry for the late reply, been busy this week. Yeah I'm having a bit of trouble with it. When the createBase function is called, many of the variables (x3d, scene, other default parameters) are undefined. Not sure exactly why they're not available, I will spend some time today to figure out what's the issue. I believe my createBase function definition is missing something. I'll let you know after today if you can look at it to see what I'm doing wrong.

image

mo-pat commented 3 years ago

Hey @jamesleesaunders could I ask for some input. I'm still stuck. I'm expecting the component I made (createBase) in component.js to have access to the variables in the methods calling createBase but that's not the case. Not sure why that's not the case here.

jamesleesaunders commented 3 years ago

@mo-jinbuu sure, I'll take a look, thanks for having a go. I'll get back to you later.

jamesleesaunders commented 3 years ago

Hi @mo-jinbuu Wow! that was more tricky than I thought. I have now fixed createBase and it now works in the examples also.

In the end I re-jigged the code which created the 'x3d' element. I also added additional parameters (classed, width, height, debug) to the createBase function signature.

    const createBase = (selection, layers, classed, width, height, debug) => {

This is not ideal as it means this function now receives 6 parameters (my IDE advises functions should have max of 3 parameters. But I am not that bothered because it now works).

Let me know if you have any other ideas or improvements otherwise I am now happy for this to be merged into master 👍

Thanks again, J

mo-pat commented 3 years ago

Hey @jamesleesaunders looks good! I was also thinking of passing all the default parameters as arguments but didn't know how to return the scene. Anyway thanks for the help I appreciate it.

I think it's good to be merged, I don't have any other suggestions/improvements so far. If you can, can you add the hacktoberfest-accepted label on this PR before merging? Taking part of hacktoberfest and I would appreciate it as it will count towards my progression.

Again thanks for the help, first time committing to open source projects. Will star and follow this repository!