-
Notifications
You must be signed in to change notification settings - Fork 74
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
Fixes #3291 : add a 'foldable' attribute, with true or false value, that... #124
Fixes #3291 : add a 'foldable' attribute, with true or false value, that... #124
Conversation
<table class="directiveSectionDef"> | ||
{ childrenXml } | ||
</table> | ||
</fieldset> | ||
</td></tr> | ||
</td></tr> ++ Script(JsRaw(""" function %s { %s } """.format(methodName, changeVisibility.toJsCmd))) |
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.
That means that all section will have there own Ajax callback written here, but it's essentially the same code.
Couldn't we have only ONE visibilityCallBack, taking the section ID in parameter, and in the backend a traversal from root section to find the matching one ?
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 didn't find out how to do it.
The visibilityCallBack need to know the section that is manipulated to change it, and that was what stuck me.
If you have suggestions I'd be happy to hear them
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.
SHtml.ajaxCall allows to execute some JS (its first arg) and pass the result as a parameter of its second paramter (hence the String => JS signature).
So I would remove the sectionId parameter of visibilityCallBack, then give as first param of ajaxCall the correct JS to get the section ID ( something around $(this).closest("[id]").id ), and barelly change visibilityCallBack JS function to use its v parameter in place of the sectionId one.
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's not the id that's a problem for me, but really mapping the id -> SectionField, because the list of SectionField changes dynamically (adding or removing sections with add or delete), and I'm really afraid it would lead to unexpected behaviour.
I can remove the sectionId from the visibilityCallback if you want, but I'm not sure that's the quite of improvement you were looking for
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.
Ok, there is certainly an other way to do it, but perhaps it's just to much hassle for not so much. Let it stay like that.
RT done ! |
…r low value,that determines if a section should be displayed by default or not
…dable_sections Fixes #3291 : add a 'foldable' attribute, with true or false value, that...
... determines if a section should be folded or not