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

feat(bigbluebutton-integration): features to support integration with bigbluebutton's recent h5p plugin #156

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

GuiLeme
Copy link

@GuiLeme GuiLeme commented Jul 24, 2024

Introduction

Hello guys! I know you are busy so I'm summing up everything that this PR do and if you find it interesting or wants to know more details, I invite you continue the reading.

What does this PR do (Summary)?

  • Update some libraries;
  • Update cypress ci actions to match new versions of libraries;
  • Update node versions to the currently supported or maintained ones;
  • Added saveFunctionCallback to get the current state of the h5p activity directly;
  • Created a way of passing content.json and h5p.json as an argument (in a case it is not passed the flow will follow as it was);

More

Hello, guys, I am a core developer at Bigbluebutton (an open-source video conferencing system for online learning), and in the past few months we've been developing our plugins system and with that we started making an integration with H5P. So for rendering the activity we decided to use h5p-standalone, but we had to make some changes to it in order to fit some of our needs.

Here we have the work we've done so far, we wanted to share with you so that it would contribute with the h5p community (we tweaked everything in order to fit a more generic purpose and not only ours), and also it would give us some directions whether we are heading the right path, since you are more experienced than us in this matter! (Thanks in advance for whatever comments or suggestions you might do, all are appreciated and welcome).

Thank you all again and if any comments or questions arise, please ping me here in the comments section so we can discuss.

@0xMurage
Copy link
Collaborator

Thank you for your first-time contribution to the project!

Comment on lines +229 to +230
if ( H5PIntegration.saveFunctionCallback instanceof Function ||
typeof H5PIntegration.saveFunctionCallback === 'function') H5PIntegration.saveFunctionCallback(state);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changes to files in the vendor directory should be done on the upstream package, as this makes the maintenance and updating of those dependencies much easier going forward without having to manually carry over changes, thereby reducing technical debt over time.

Comment on lines +216 to +218
H5PJsonContent = <H5PPackageDefinition>(await getJSON(`${h5pJsonPath}/h5p.json`, options?.assetsRequestFetchOptions).catch((e) => {
console.log('Error while trying to fetch h5p json content: ', e, `${h5pJsonPath}/h5p.json`, options?.assetsRequestFetchOptions)
}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Catching an error without either rethrowing it or breaking from the function will cause execution to continue, which will lead to unexpected behavior or errors further down the code.

@0xMurage
Copy link
Collaborator

It would be great if in future you could consider breaking PRs into smaller, more focused PR.

for example, this PR would have been broken into

  • upgrading the Cypress tests and CI/CD pipeline
  • adding the feature to load the H5P config from JSON in local memory
  • adding saveFunction callback

That way, the review process can be more focused and efficient. Please keep this in mind for your next contribution. We appreciate you taking the time to improve the project!

@0xMurage
Copy link
Collaborator

and also it would give us some directions whether we are heading the right path,

The feature of loading the H5P config from a JSON in memory is a great addition and very much welcomed. You're definitely on the right path.

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.

4 participants