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
The $let widget #6148
The $let widget #6148
Conversation
core/modules/widgets/let.js
Outdated
/* | ||
Inherit from the base widget class | ||
*/ | ||
LetWidget.prototype = Object.create(Widget.prototype); |
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 pattern we follow in all the other widgets is to call new Widget()
here, which obviates the need to call the constructor later.
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.
Shall I change $vars over too? I copied this code from there.
core/modules/widgets/let.js
Outdated
*/ | ||
LetWidget.prototype.refresh = function(changedTiddlers) { | ||
var changedAttributes = this.computeAttributes(); | ||
if(Object.keys(changedAttributes).length) { |
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 usual pattern in the core is if($tw.utils.count(changedAttributes) > 0)
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'll change this in vars too, which doesn't use this pattern.
core/modules/widgets/let.js
Outdated
}; | ||
|
||
LetWidget.prototype.getVariableInfo = function(name,options) { | ||
// Special handling: If this variable exists in this very $vars, we can |
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.
Should it be $vars
or $let
|
||
! Introduction | ||
|
||
The <<.wid let>> widget allows multiple variables to be set in one operation. In some situations it can result in simpler code than using the more flexible <<.wlink SetWidget>> widget. It differs from the <<.wlink VarsWidget>> widget in that variables you're defining may depend on earlier variables defined within the same <<.wid let>>. |
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.
IMO should start with: <<.from-version "5.2.1">>
The ...
Exciting stuff, thank you @flibbles |
Thanks @flibbles
Great, I think that's the right position for the moment. |
Applied changes you guys recommended. Let me know if any of you see anything else. |
core/modules/widgets/let.js
Outdated
*/ | ||
LetWidget.prototype.render = function(parent,nextSibling) { | ||
// Call the constructor | ||
Widget.call(this); |
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 believe this call is now superfluous, likewise in $vars, otherwise it looks good.
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.
done.
Also improved $let test
Looks good @flibbles. Thank you. This will make me upgrade the wiki I am currently working on to the pre-release as soon as it is merged. |
@Jermolene look forward to your thoughts on this when you have the chance to review. |
Thanks @flibbles, great stuff. Apologies for the delay, I wanted to properly check this out. |
Here is the $let widget, which takes advantage of orderedAttributes in order to allow for interdependent variables, as well as multiple definitions of the same variable.
I made a few design decisions while making this:
Otherwise, this does everything it's promised to do. It took some special care to make sure it doesn't refresh unnecessarily, but it should all be good now. Also included some documentation (and I touched up varsWidget documentation to use the .wid and .wlink macros when appropriate).
Let me know what ya'll think.