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: Include check for model.get('_type') !== 'block' when filtering models (fixes #197) #198

Closed
wants to merge 1 commit into from

Conversation

joe-allen-89
Copy link
Contributor

adaptlearning/adapt-contrib-trickle#221

Fix

@joe-allen-89 joe-allen-89 self-assigned this May 13, 2024
Copy link
Contributor

@danielghost danielghost left a comment

Choose a reason for hiding this comment

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

Seems to only be an issue when trickle "_onChildren": false for the article, as that is when the trickle-button is included in this._originalChildModelsStore and then subsequently removed in the filter.

This works, but is model.get('_type') !== 'block' the clearest way to maintain the trickle-button in the list? Should it be more explicit regarding the intention, e.g. model.get('_component') === 'trickle-button' or model.get('_isTrackable') === false. Maybe a comment would suffice?

@oliverfoster
Copy link
Member

Why is it failing the data.hasId() check? The trickle button should be in the data collection still...

Perhaps I never added them to data? https://github.com/adaptlearning/adapt-contrib-trickle/blob/2c550355f0e98f0d50d07f8fdf9614ccfc94ac6d/js/models.js#L294-L319 Weird.

Could you try with a data.add(trickleButtonModel); here as an alternative please? @joe-allen-89

@joe-allen-89
Copy link
Contributor Author

@danielghost - I used model.get('_type') !== 'block' as it's the same method used further down to identify trickle, but yes a comment to make more obvious would have helped.

@oliverfoster - Tbh I assumed there was a reason you'd kept it out the data, adding the trickle to data also works, I'll close this PR, transfer issue and raise a new PR on trickle if that's your preferred option.

@oliverfoster
Copy link
Member

I suspect I was probably just cautious about it initially, but there's really no reason not to.

Branching clears cloned models up if it comes to remove them https://github.com/adaptlearning/adapt-contrib-branching/blob/036b75fef296c5e09446ef09dfa3969f29eb695e/js/BranchingSet.js#L313

@joe-allen-89
Copy link
Contributor Author

Closing PR as transferring issue to trickle for alternative fix.

@joe-allen-89 joe-allen-89 deleted the issue/197 branch May 15, 2024 09:05
@joe-allen-89
Copy link
Contributor Author

Alternative fix implemented, PR raised - adaptlearning/adapt-contrib-trickle#222

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trickle button not showing when assessment has been completed
3 participants