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.

Sign up for GitHub

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-5467 return null when resume state is empty #86

Merged
merged 1 commit into from
Sep 20, 2023
Merged

JI-5467 return null when resume state is empty #86

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/scripts/h5p-crossword-content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,10 +280,15 @@ export default class CrosswordContent {
return;
}

const cells = this.table.getAnswers();
const focus = this.table.getFocus();
if (!cells.find((item) => item !== undefined) && typeof focus.position.row === 'undefined') {
Copy link
Owner

Choose a reason for hiding this comment

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

Why not use getAnswerGiven (in the main instance class, here h5p-crossword.js) that's defined in the H5P question type contract and that should be implemented in most content types? It is supposed to provide logic that tells you whether an answer was given by the user. No need to check how that specific information can be retrieved by a content type and then needing to add extra implementation to getCurrentState for every content type. That might help to keep this handling uniform across content types, easier to change later on.

The one thing that would not be covered is if when the user added an answer but then wiped the slate clean again - getAnswerGiven would still return true. Don't know what exactly the rules are here ...

Why are you checking for the focus position here? Does setting focus also count as relevant for the restart button? One can well argue: yes. Just want try to find out what the non-written guideline is.

And a little nitpicking now:
if (cells.every((item) => item === undefined)) feels easier to understand without a double negation. Make that if (!cells.some((item) => item !== undefined)) if you want to exit early without having to always traverse all the items.

A comment would also be helpful, as this expected behavior in not obvious IMHO, cmp. otacke/h5p-sort-paragraphs@03785a6

Copy link
Contributor Author

@devland devland Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why not use getAnswerGiven

If the user, for example, moves to a slide in course presentation, then that interaction should be restored on page refresh and getAnswerGiven does not help us in this case. It also would forever return true even if the user deletes the input so, for consistency, we're going with a custom approach for every content type library.

It would have been nice to have dealt with this only once in a top level library but such a solution has not presented itself. :)

Why are you checking for the focus position here?

It's part of the to be restored user input just like how moving to a new slide is in CP.
It might be considered a bit overzealous but it's useful for accessibility reasons.

And a little nitpicking now:
if (cells.every((item) => item === undefined)) feels easier to understand without a double negation. Make that if (!cells.some((item) => item !== undefined))

cells.every would iterate through all entries and that isn't required.
Array.find stops iterating & returns the first item that satisfies the given condition.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I fail to see what getAnswerGiven in Crossword has to do with CoursePresentation. But yes, getAnswerGiven in CoursePresentation may not be what you're looking for there. And again: There's no publicly available documentation, and I don't know the hard rules that should determine whether that H5P.com restart button should show or not. That's why I asked about the focus, for instance. If the requirement is just some letter entered into the grid or text input fields, then that is what getAnswerGiven will tell you here.

I listed cells.every as a way to prevent double negation and to make the code clearer as mentioned, not to speed things up, and cells.some as an alternative to cells.every - also more clear than find, but with the same traversal logic that doesn't necessarily look at all the items.

return;
}
return {
crosswordLayout: this.crosswordLayout,
cells: this.table.getAnswers(),
focus: this.table.getFocus()
cells,
focus
};
}

Expand Down