You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
The intention of this code is to stop object.template == name as default if a template is specified.
The better solution is to add "template": "gmcq" to the gmcq definition.
The reason will be displayed to describe this comment to others. Learn more.
My point is that the intention breaks course with the GMCQ, the fix would be not simply check if the property exist but also compare the content of the property.
@hrajotia it is good solution as well but this also needs to be added to any other components that extends others, my change is single point one fix
The reason will be displayed to describe this comment to others. Learn more.
the component register function is the only place in adapt where templates do not need to be explicitly declared and it's really bad for clarity. all other plugins define their templates and realistically, the components should too. giving more power to the register function is not really the direction we should be heading in, although it does indeed solve a temporary issue.
The reason will be displayed to describe this comment to others. Learn more.
I submitted this change so that register would play a bit nicer with extensions that inherited from other extensions. Previously, you were forced to create a template for every extension, rather than use an existing template, and just extend the functionality.
I agree with @oliverfoster about removing the line altogether.
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it has a bug as the object can have inherited template property, test with GMCQ for instance, it will have mcq already.
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried making a PR myself to the Grunt Handlebars module: gruntjs/grunt-contrib-handlebars#147
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this would be better option:
if(!object.template || object.template != name)
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention of this code is to stop
object.template == name
as default if a template is specified.The better solution is to add "template": "gmcq" to the gmcq definition.
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see - https://github.com/adaptlearning/adapt-contrib-gmcq/pull/87/files
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @hrajotia
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that the intention breaks course with the GMCQ, the fix would be not simply check if the property exist but also compare the content of the property.
@hrajotia it is good solution as well but this also needs to be added to any other components that extends others, my change is single point one fix
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please apply the fix himanshu has listed above ^ it's not in master yet but is in the develop of gmcq.
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the component register function is the only place in adapt where templates do not need to be explicitly declared and it's really bad for clarity. all other plugins define their templates and realistically, the components should too. giving more power to the register function is not really the direction we should be heading in, although it does indeed solve a temporary issue.
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would prefer line 83 to go missing entirely.
a7af2e3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I submitted this change so that register would play a bit nicer with extensions that inherited from other extensions. Previously, you were forced to create a template for every extension, rather than use an existing template, and just extend the functionality.
I agree with @oliverfoster about removing the line altogether.