parallaxinc / solo

BlocklyProp without the wires
MIT License
5 stars 6 forks source link

Refactor graphing code into a class #356

Open MatzElectronics opened 4 years ago

MatzElectronics commented 4 years ago

Just like we did with the terminal

pjewald commented 4 years ago

Please, not like terminal. Use the outline I sent to you offline.

MatzElectronics commented 4 years ago

Not like #200 you mean, right? I thought the way propterm.js got done would fit this outline well.

pjewald commented 4 years ago

All of the graphing bits should reside in a single file, more if it needs multiple classes to make it all modular. The PropTerm class is missing the Class decorator, but otherwise behaves as a single unit. Like I outlined offline, has to be built in isolation so we can test it. Let me know if it has or will need dependencies and we will work through those details.

MatzElectronics commented 4 years ago

It will need 1 dependency outright - chartist.js (library).

Otherwise, it "sort of" needs access to the fields in the blocks - specifically the graphing blocks, but only on open, for the purpose of setting up the graph.  This is why I did #370.

I can envision a buildPropGraphSettings module and a propGraph module and I'm confident I can completely isolate everything using a few callbacks.

Once it is isolated, we can refactor a substantial chunk of blocklyc.js to remove what is basically redundant code between the graph and the terminal and just add a couple of conditions to pipe characters to the correct module (graph or terminal) for display.

MatzElectronics commented 4 years ago

As for the dependency - graphing is the only thing that uses chartist.js

zfi commented 4 years ago

So chartist is going to be a real challenge. It has not seen an update in three years and does not play nicely with current versions of node and npm. All the examples I find are using a different implementation than ES2015 modules. Basically using requires() instead of import...

The "sort of" needs access to blocks can be viewed as the blocks need to provide callbacks to use the charist package. We should avoid chartist reaching directly into blocks because that will create a dependency outside of the blocks. Maybe I don't understand it correctly.

It's really looking like this is going to have to remain script code unless there is a better mousetrap out there. So it can still be built as one or more classes. It just can't be a module.

MatzElectronics commented 4 years ago

I'm not opposed to switching engines. Here's another: https://www.chartjs.org/docs/latest/charts/line.html

MatzElectronics commented 4 years ago

The blocks dont talk to chartist directly. When the graph gets opened, it needs to know it's scaling, axes, type (line or scatter), etc. That info can be extracted from the blocks first, packaged into a settings object, and then passed into chartist (or whatever other library is used)

zfi commented 4 years ago

Chartjs looks impressive and easy to implement.

import Chart from 'chart.js';
var myChart = new Chart(ctx, {...});
zfi commented 4 years ago

So the relevant blocks have data that is of interest to the charting system? Is there something that knows how to harvest this data from the blocks? Maybe that is the extractor object. That would nicely decouple the graphing package from the source data.

MatzElectronics commented 4 years ago

That's exactly what the changes in #370 decoupled and set up.

When we chat over the weekend, I'll detail how it all works. It's not complicated, but I'm the only one who has ever touched that code, so I understand why it's a black box.

MatzElectronics commented 4 years ago

sorry, one more - Any graph needs two things, settings and data. The settings come from the blocks. The data comes directly from the propeller device via the serial port.

The only practical difference between a graph and a terminal is that a terminal doesn't need settings (at least not to the extent that a graph does)