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

Fix: Call storeUserAnswer on item change (fixes #493) #494

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster commented Jan 31, 2024

fixes #493

Fix

  • Call storeUserAnswer on item change

Copy link
Contributor

Choose a reason for hiding this comment

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

👀

Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

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

@oliverfoster if we're firing this.storeUserAnswer on setActiveItem should we also we remove it from checkCompletionStatus as it's currently firing twice when an child's _isVisited changes?

@oliverfoster
Copy link
Member Author

oliverfoster commented Feb 2, 2024

@oliverfoster if we're firing this.storeUserAnswer on setActiveItem should we also we remove it from checkCompletionStatus as it's currently firing twice when an child's _isVisited changes?

Hmm, probably not. One could change the active item without calling setActiveItem using the item api

export default class ItemModel extends LockingModel {
defaults() {
return {
_isActive: false,
_isVisited: false,
_score: 0
};
}
reset() {
this.set({ _isActive: false, _isVisited: false });
}
toggleActive(isActive = !this.get('_isActive')) {
this.set('_isActive', isActive);
}
toggleVisited(isVisited = !this.get('_isVisited')) {
this.set('_isVisited', isVisited);
}

It's a shame it's called twice. Perhaps setActiveItem in core is not the place for it? Maybe I should just move it to the narrative pr?

This is needed because checkCompletionStatus only saves before completion, not after it. With presentation components it's possible to change the active item even once the component is complete.

Or maybe it should just save whenever the children change? Detached from completion and a specific change api?

@oliverfoster
Copy link
Member Author

I'm going to move this to narrative for the time being I think.

oliverfoster added a commit to adaptlearning/adapt-contrib-narrative that referenced this pull request Feb 2, 2024
@joe-allen-89
Copy link
Contributor

@oliverfoster good points I'd not thought of updating through the item, it feels like having it in core is sensible as it can be utilised by other components without duplicating.

Having it on children change does seem like a good option as it seperates it from both completion and setActiveItem, listen for change: _isActive maybe?

@oliverfoster
Copy link
Member Author

oliverfoster commented Feb 2, 2024

_isActive and _isVisited seem to be quite closely linked, _isVisited is used for checkCompletionStatus and _isActive is used for selecting the item, with narrative something can't be _isVisited unless it was once _isActive but the code allows _isVisited and _isActive to be set independently of each other, _isVisited is used to save state here, _isActive is used to keep the active item.

I've just made it save when any property changes to keep it easy.

@oliverfoster oliverfoster changed the title Fix: Call storeUserAnswer on activeItem change (fixes #493) Fix: Call storeUserAnswer on item change (fixes #493) Feb 2, 2024
@oliverfoster oliverfoster merged commit d208379 into master Feb 2, 2024
@oliverfoster oliverfoster deleted the issue/493 branch February 2, 2024 17:34
github-actions bot pushed a commit that referenced this pull request Feb 2, 2024
## [6.45.3](v6.45.2...v6.45.3) (2024-02-02)

### Fix

* Allow /#/preview/b-05 for rendering a single id (fixes #488) (#495) ([349629b](349629b)), closes [#488](#488) [#495](#495)
* Call storeUserAnswer on item change (fixes #493) (#494) ([d208379](d208379)), closes [#493](#493) [#494](#494)
@oliverfoster
Copy link
Member Author

🎉 This PR is included in version 6.45.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

joe-allen-89 added a commit to adaptlearning/adapt-contrib-narrative that referenced this pull request Feb 5, 2024
* Fix: Keep active item position on save/restore (fixes #295)

* Fix: storeUserAnswer on setActiveItem

Moved from adaptlearning/adapt-contrib-core#494

* Revert

* FW bump for core dependency

---------

Co-authored-by: joe-allen-89 <85872286+joe-allen-89@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ItemsComponentModel not possible to store active item for restoration later
4 participants