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

JI-5501 Always clear previousState in resetTask, #93

Merged

Conversation

makmentins
Copy link
Contributor

reset DOM only if loaded

@otacke
Copy link
Owner

otacke commented Oct 3, 2023

@makmentins Will probably look at this tomorrow. I am not at my desk today.

@otacke
Copy link
Owner

otacke commented Oct 4, 2023

@makmentins What's the goal here? The pull request is not too verbose :-) I assume you're trying to prevent the previous state from being loaded upon refresh if resetTask() was called. That may work on H5P.com - it may erase the entry in database if getCurrentState returns undefined (don't know, can't look into the code), but other H5P integrations will not. Cmp. otacke/h5p-sort-paragraphs#42 and Erik's confirmation at otacke/h5p-sort-paragraphs#42 (comment) and amendmend for Sort the Paragraphs at otacke/h5p-sort-paragraphs#43

Always set isAnswered to false when resetting
@makmentins
Copy link
Contributor Author

Yep, I we're still making sure that all of the content is completely reset when resetTask is called, in this case the extra conditions were necessary to ensure that it works correctly even if DOM is not loaded (when Essay is a part of a compound content type).

I'm not entirely sure returning undefined in getCurrentState would ever be a problem, but it's a good observation nonetheless, we should keep it consistent across content types, so I pushed a commit for doing that.
Additionally, same as for state, isAnswered should probably also always be reset when resetting task, so I've moved it outside of the condition for DOM manipulations

@otacke
Copy link
Owner

otacke commented Oct 4, 2023

That should do the trick! The expected behavior should be clearly documented in https://h5p.org/documentation/developers/contracts#guides-header-5 It currently does not mention states that are stored in the user data table of the H5P integration.

@otacke
Copy link
Owner

otacke commented Mar 7, 2024

Whoops, never merged that in, sorry.

@otacke otacke merged commit cdea29b into otacke:master Mar 7, 2024
1 check passed
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