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.

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

Communication system for embedded WW problems #16

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

drdrew42
Copy link
Member

@drdrew42 drdrew42 commented Jun 22, 2023

This PR builds off of #15

Embedded WeBWorK problems now emit messages on supported events:

type status id
webwork.interaction.attempt [0-1] score na
webwork.interaction.hint active/collapsed knowl-uid-#
webwork.interaction.solution active/collapsed knowl-uid-#
webwork.interaction.focus na AnSwEr####
webwork.interaction.blur na AnSwEr####
webwork.interaction.toolbar na toolbar_id

The split page editor is listening to the parent window and logs all interactions to the console. Open the developer tools to see this PR in action!

@drdrew42 drdrew42 changed the base branch from main to develop June 22, 2023 22:48
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.

This review is of #14, #15, and #16. I initially looked at #14 and saw things that didn't look right, and then looked at #16 and saw that those things were fixed. In retrospect I probably should have reviewed #15, and then #16 should have been reviewed separately, but I don't want to redo the comments in the code.

In any case, everything generally looks good. There are some clean up issues though.

The package-lock.json file in the renderer root directory should be deleted.

Comment on lines +113 to +114
warn "*** Configuration error: baseURL should not end in a slash\n" if $ENV{baseURL} =~ s!/$!!;
warn "*** Configuration error: baseURL should begin with a slash\n" unless $ENV{baseURL} =~ s!^//!/!;
Copy link
Member

Choose a reason for hiding this comment

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

These warnings are displayed if $ENV{baseURL} is the empty string. I think that is supported, so some care is needed to prevent those warnings in that case.

Comment on lines +61 to +62
# The second element of each array in the following is whether or not the file is a theme file.
# customize source for bootstrap.css
Copy link
Member

Choose a reason for hiding this comment

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

The comment about the second element determining if the file is a theme file should be deleted. It isn't relevant for the standalone renderer.

The second comment seems out of place.

Comment on lines +92 to +93
# The second element of each array in the following is whether or not the file is a theme file.
# The third element is a hash containing the necessary attributes for the script tag.
Copy link
Member

Choose a reason for hiding this comment

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

These comments should be updated as the part about theme files is not relevant.

@@ -0,0 +1,123 @@
/* WeBWorK Online Homework Delivery System
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this file or pgeditor.css are used anywhere. Are they needed?

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.

2 participants