Skip to content

Plotly3D code improvements.#787

Merged
pstaabp merged 5 commits intoopenwebwork:developfrom
somiaj:plotly3D
Mar 21, 2023
Merged

Plotly3D code improvements.#787
pstaabp merged 5 commits intoopenwebwork:developfrom
somiaj:plotly3D

Conversation

@somiaj
Copy link
Contributor

@somiaj somiaj commented Mar 13, 2023

Add a local copy of plotly.js version 2.18.2 to the collection of node_modules installed on the server to be able to host the javascript locally instead of using a CDN.

Use PG->getUniqueName('plotly3D') to get a unique string for the variable names instead of relying on incrementing the variables based on the number of graphs used. This allows this to avoid conflicts in tests or pages that have more than a single problem on the screen.

  Add a local copy of plotly.js version 2.18.2 to the collection
  of node_modules installed on the server to be able to host the
  javascript locally instead of using a CDN.

  Use PG->getUniqueName('plotly3D') to get a unique string for
  the variable names instead of relying on incrementing the variables
  based on the number of graphs used. This allows this to avoid
  conflicts in tests or pages that have more than a single problem
  on the screen.
@drgrice1
Copy link
Member

@somiaj: I added a pull request to this branch with some improvements. The main thing is that there is no need to use the getUniqueName generated id for variable and function suffixes. This reduces the size of the generated javascript considerably, since that id is rather lengthy. The only place that id is used with that pull request is for the unique html id on the containing div.

@drgrice1
Copy link
Member

I added one more small change to my pull request to this branch. That is I updated the method used for loading the javascript to work for the webwork3 rendering approach in which the javascript runs after the page has loaded. As such the DOMContentLoaded event has already run and the plotly javascript will also have already been loaded. So for that the initialization method in this macro needs to just run.

drgrice1 and others added 2 commits March 13, 2023 08:55
…ames.

Instead scope those sections properly in the javascript so that static
variable names can be used.  The only thing the unique id is really
needed for is to have a unique html id for the containing div.  This
makes the generated javascript smaller since that generated id is quite
long.

Also fix the funcType 'perl' option.

The funcType 'perl' and funcType 'data' need better documentation.

Also update the package-lock.json file.  Something wasn't done right
there as running `npm install` modifies the lock file.

Finally, update the loading method in javascript to work for webwork3
and similar rendering techniques where the javascript will be added to
the page after the DOMContentLoaded event has already run.
Improvements to the plotly3D.pl macro.
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for making the requested changes.

@pstaabp
Copy link
Member

pstaabp commented Mar 16, 2023

Just for documentation, these are additional improvements to #728

@pstaabp
Copy link
Member

pstaabp commented Mar 16, 2023

Heads up, but this will need an npm ci and not just a generate-assets.js for testing (and after this is merged into develop) since there is a new js package added.

@pstaabp
Copy link
Member

pstaabp commented Mar 17, 2023

After doing the above things, including clearing the cache, I'm getting the following plot for the example you posted on the #728:

image

It appears that plotly.min.js is loading correctly, but doesn't seem to be rendering the plot.

@drgrice1
Copy link
Member

@pstaabp: Which example from #728 are you using? You aren't using the example in the original comment are you? Make sure you are using the updated example there.

@drgrice1
Copy link
Member

Scroll down, and use the last one in the comments there.

@pstaabp
Copy link
Member

pstaabp commented Mar 17, 2023

Of course I used an old problem. The updated one at the bottom of #728 works great.

@drgrice1
Copy link
Member

@somiaj: Probably the best way is to run the bin/dev_scripts/generate-ww-pg-pod.pl script. That generates the POD in the same way that the POD is generated for the wiki (as that is the script that generates the POD for the wiki).

@somiaj
Copy link
Contributor Author

somiaj commented Mar 18, 2023

Can't seem to get the script to work. I installed libpod-parser-perl (which caused the script to run without errors) and then ran:

./generate-ww-pg-pod.pl -v --pg-root=/opt/webwork/pg --webwork-root=/opt/webwork/webwork2 -o ~/POD

And it creates three files, ~/POD/index.html, ~/POD/pg/index.html, and ~/POD/webwork2/index.html. Unsure what I'm missing to make it traverse through the files to build the html docs.

  Better describe how the various funcType work, including examples.
  Fix some typos.
@drgrice1
Copy link
Member

You don't need to pass the --pg-root and --webwork-root options if the usual PG_ROOT and WEBWORK_ROOT environment variables are set.

I am not sure what is going wrong for you. If I cut an paste the command line that you posted it runs fine, and generates all of the webwork2 and pg POD.

The only thing I can think of is to check that all of the perl modules are available that are used by the script. Those are Pod::Usage, Getopt::Long, Pod::Simple::XHTML, File::Find, File::Path, IO::File, Pod::Find, File::Basename, and POSIX.

@somiaj
Copy link
Contributor Author

somiaj commented Mar 18, 2023

Okay, figured it out. The parser doesn't like symbolic links for /opt/webwork/pg and /opt/webwork/webwork2.

@pstaabp pstaabp merged commit 3e0559a into openwebwork:develop Mar 21, 2023
@somiaj somiaj deleted the plotly3D branch March 21, 2023 16:48
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.

3 participants