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

Add actions parameter to radio-widget #5026

Closed
wants to merge 134 commits into from
Closed
Show file tree
Hide file tree
Changes from 133 commits
Commits
Show all changes
134 commits
Select commit Hold shift + click to select a range
090e812
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
5f35bab
make eslint, jslint happy
pmario Jan 4, 2020
4d610fa
Merge remote-tracking branch 'jermolene/master'
pmario Jan 13, 2020
4f68732
Merge remote-tracking branch 'jermolene/master'
pmario Jan 29, 2020
c0d4507
Merge remote-tracking branch 'jermolene/master'
pmario Jan 30, 2020
19e9eff
Merge remote-tracking branch 'jermolene/master'
pmario Jan 31, 2020
8f5b635
Merge remote-tracking branch 'jermolene/master'
pmario Feb 10, 2020
15f79e1
Merge remote-tracking branch 'jermolene/master'
pmario Feb 16, 2020
789e6fb
Merge remote-tracking branch 'jermolene/master'
pmario Mar 14, 2020
f39eaf1
Merge remote-tracking branch 'jermolene/master'
pmario Mar 19, 2020
b97cf82
Merge remote-tracking branch 'jermolene/master'
pmario Mar 24, 2020
c1448df
Merge remote-tracking branch 'jermolene/master'
pmario Mar 28, 2020
a30852a
it shouldn't be there
pmario Mar 30, 2020
b63b602
fremove this file from my PRs
pmario Mar 30, 2020
bc192f8
Merge remote-tracking branch 'jermolene/master'
pmario Mar 31, 2020
611da36
Merge remote-tracking branch 'jermolene/master'
pmario Apr 6, 2020
9f8d930
Merge remote-tracking branch 'jermolene/master'
pmario Apr 8, 2020
f8789d7
Merge remote-tracking branch 'jermolene/master'
pmario Apr 9, 2020
ae84acd
Merge remote-tracking branch 'jermolene/master'
pmario Apr 22, 2020
cc07cf8
Merge remote-tracking branch 'jermolene/master'
pmario Apr 27, 2020
4fe0310
Merge remote-tracking branch 'origin/master'
pmario Apr 27, 2020
31ce0cf
Merge remote-tracking branch 'jermolene/master'
pmario May 5, 2020
ea29fdb
Merge remote-tracking branch 'jermolene/master'
pmario Jun 4, 2020
3c9b6f1
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
bd0b9a3
make eslint, jslint happy
pmario Jan 4, 2020
0589f60
it shouldn't be there
pmario Mar 30, 2020
34866cb
fremove this file from my PRs
pmario Mar 30, 2020
313a7ef
Merge remote-tracking branch 'origin/master'
pmario Jul 6, 2020
7d92bd0
Merge remote-tracking branch 'jermolene/master'
pmario Jul 6, 2020
83516c8
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
267b001
make eslint, jslint happy
pmario Jan 4, 2020
01555e1
it shouldn't be there
pmario Mar 30, 2020
1af3cb7
fremove this file from my PRs
pmario Mar 30, 2020
422f862
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
4ee1de0
make eslint, jslint happy
pmario Jan 4, 2020
5f51070
it shouldn't be there
pmario Mar 30, 2020
925871e
fremove this file from my PRs
pmario Mar 30, 2020
83b40e0
Merge remote-tracking branch 'jermolene/master'
pmario Aug 9, 2020
32f8666
Merge remote-tracking branch 'origin/master'
pmario Aug 9, 2020
48f51d0
Merge remote-tracking branch 'jermolene/master'
pmario Aug 10, 2020
7738c4d
Merge remote-tracking branch 'origin/master'
pmario Aug 10, 2020
ee59fb4
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
c1f3b08
make eslint, jslint happy
pmario Jan 4, 2020
faca4d7
it shouldn't be there
pmario Mar 30, 2020
393447f
fremove this file from my PRs
pmario Mar 30, 2020
2cca066
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
1ceb069
make eslint, jslint happy
pmario Jan 4, 2020
3fd944f
it shouldn't be there
pmario Mar 30, 2020
d3881b2
fremove this file from my PRs
pmario Mar 30, 2020
bf5ace9
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
264090a
make eslint, jslint happy
pmario Jan 4, 2020
0933fac
it shouldn't be there
pmario Mar 30, 2020
7358345
fremove this file from my PRs
pmario Mar 30, 2020
23f88d7
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
4a4f53a
make eslint, jslint happy
pmario Jan 4, 2020
e247e68
it shouldn't be there
pmario Mar 30, 2020
7de28db
fremove this file from my PRs
pmario Mar 30, 2020
03b697c
Merge remote-tracking branch 'origin/master'
pmario Aug 13, 2020
1708b89
Merge remote-tracking branch 'origin/master' into master
pmario Sep 4, 2020
80d6bb9
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
c57a52f
make eslint, jslint happy
pmario Jan 4, 2020
f4c8fd1
it shouldn't be there
pmario Mar 30, 2020
f8fe8d5
fremove this file from my PRs
pmario Mar 30, 2020
bf7c65d
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
d89471b
make eslint, jslint happy
pmario Jan 4, 2020
b503781
it shouldn't be there
pmario Mar 30, 2020
90662c8
fremove this file from my PRs
pmario Mar 30, 2020
2907799
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
cdcb873
make eslint, jslint happy
pmario Jan 4, 2020
145197a
it shouldn't be there
pmario Mar 30, 2020
d5c03e5
fremove this file from my PRs
pmario Mar 30, 2020
dbf96a9
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
00c0b7f
make eslint, jslint happy
pmario Jan 4, 2020
e0dd0d2
it shouldn't be there
pmario Mar 30, 2020
784329a
fremove this file from my PRs
pmario Mar 30, 2020
283c844
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
e86bc01
make eslint, jslint happy
pmario Jan 4, 2020
38fb557
it shouldn't be there
pmario Mar 30, 2020
58a009a
fremove this file from my PRs
pmario Mar 30, 2020
e249760
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
2413d34
make eslint, jslint happy
pmario Jan 4, 2020
d37e21a
it shouldn't be there
pmario Mar 30, 2020
ae0e266
fremove this file from my PRs
pmario Mar 30, 2020
6686fdd
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
29be731
make eslint, jslint happy
pmario Jan 4, 2020
6259d87
it shouldn't be there
pmario Mar 30, 2020
9af4fc5
fremove this file from my PRs
pmario Mar 30, 2020
14fd2f3
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
0de7341
make eslint, jslint happy
pmario Jan 4, 2020
5a8cbc1
it shouldn't be there
pmario Mar 30, 2020
0149239
fremove this file from my PRs
pmario Mar 30, 2020
067a76e
Don't override browser selection colours by default
Jermolene Sep 25, 2020
95b99ed
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
4209280
make eslint, jslint happy
pmario Jan 4, 2020
51525a1
it shouldn't be there
pmario Mar 30, 2020
36ab8a8
fremove this file from my PRs
pmario Mar 30, 2020
0d9c15d
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
c37cf0c
make eslint, jslint happy
pmario Jan 4, 2020
07d8a39
it shouldn't be there
pmario Mar 30, 2020
672868e
fremove this file from my PRs
pmario Mar 30, 2020
00b2c14
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
4f95321
make eslint, jslint happy
pmario Jan 4, 2020
42167b4
it shouldn't be there
pmario Mar 30, 2020
b25ff28
fremove this file from my PRs
pmario Mar 30, 2020
7cb6a69
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
5e67097
make eslint, jslint happy
pmario Jan 4, 2020
d426385
it shouldn't be there
pmario Mar 30, 2020
bf55dbe
fremove this file from my PRs
pmario Mar 30, 2020
e77cef9
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
3c10009
make eslint, jslint happy
pmario Jan 4, 2020
fa72a58
it shouldn't be there
pmario Mar 30, 2020
8436970
fremove this file from my PRs
pmario Mar 30, 2020
f84a5f5
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
f2e5744
make eslint, jslint happy
pmario Jan 4, 2020
a223738
it shouldn't be there
pmario Mar 30, 2020
8a15f34
fremove this file from my PRs
pmario Mar 30, 2020
4624788
add a new-line before the log text to increase readability of the tes…
pmario Jan 4, 2020
46fd2a5
make eslint, jslint happy
pmario Jan 4, 2020
99a6bf3
it shouldn't be there
pmario Mar 30, 2020
958fa42
fremove this file from my PRs
pmario Mar 30, 2020
6c3bff4
Merge branch 'master' of https://github.com/Jermolene/TiddlyWiki5 int…
pmario Oct 20, 2020
b98e1ca
Merge branch 'master' of github.com:pmario/TiddlyWiki5 into master
pmario Oct 20, 2020
dcf722b
Merge branch 'master' of https://github.com/Jermolene/TiddlyWiki5 int…
pmario Oct 26, 2020
4ac2495
Merge branch 'master' of https://github.com/Jermolene/TiddlyWiki5 int…
pmario Oct 27, 2020
86bf726
Merge branch 'master' of https://github.com/Jermolene/TiddlyWiki5 int…
pmario Nov 8, 2020
c9a65f3
Merge branch 'master' of https://github.com/Jermolene/TiddlyWiki5 int…
pmario Nov 11, 2020
9de7442
WIP initial test radio actions
pmario Nov 12, 2020
3968ac2
WIP first try to add actions to radio buttons
pmario Nov 12, 2020
1dbe8f7
WIP add tiddler and field variables
pmario Nov 12, 2020
bb93ddc
WIP code refactoring to make it consistent
pmario Nov 13, 2020
50ffeed
add parameter prefix "attr-" and update refresh handling
pmario Nov 15, 2020
89ad523
add `actionValue` to the action variables
pmario Nov 17, 2020
65cbba3
if this.radioTitle is changed, it nees a refreshSelf(), otherwise the…
pmario Nov 22, 2020
a00fc21
simplify widget refres code
pmario Nov 22, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 22 additions & 8 deletions core/modules/widgets/radio.js
Expand Up @@ -13,7 +13,6 @@ Set a field or index at a given tiddler via radio buttons
"use strict";

var Widget = require("$:/core/modules/widgets/widget.js").widget;

var RadioWidget = function(parseTreeNode,options) {
this.initialise(parseTreeNode,options);
};
Expand All @@ -37,8 +36,8 @@ RadioWidget.prototype.render = function(parent,nextSibling) {
// Create our elements
this.labelDomNode = this.document.createElement("label");
this.labelDomNode.setAttribute("class",
"tc-radio " + this.radioClass + (isChecked ? " tc-radio-selected" : "")
);
"tc-radio " + this.radioClass + (isChecked ? " tc-radio-selected" : "")
);
this.inputDomNode = this.document.createElement("input");
this.inputDomNode.setAttribute("type","radio");
if(isChecked) {
Expand Down Expand Up @@ -83,9 +82,24 @@ RadioWidget.prototype.setValue = function() {
};

RadioWidget.prototype.handleChangeEvent = function(event) {
var variables = Object.create(null);
if(this.inputDomNode.checked) {
this.setValue();
}
// Trigger actions. Use variables = {key:value, key:value ...}
if(this.radioActions) {
$tw.utils.each(this.attributes,function(val,key) {
if(key.charAt(0) !== "$") {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Jermolene @pmario So is the intention that all attributes starting with $ are a reserved name space for future attributes that we might want to add to the widget? By allowing users to use arbitrary attribute names, in effect every attribute name becomes a reserved one, as it could clash with widget attributes that we introduce in the future.

At the moment it is just action-widgets that have attributes starting with $. This is confusing enough as it is but at least users can remember the rule that non-action widget attribute names do not start with $.

It would be quite confusing if suddenly any widget could have attribute names that may or not start with $. Especially if the widget previously has many attributes with no $ prefix and then suddenly has one new one introduced with a $ prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you suggest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could imagine, that "user_attributes" start with an underscore. eg: _user-attribute and reserved-for-core and $reserved-for-core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or: __attribute with 2 underscores

Copy link
Contributor

@saqimtiaz saqimtiaz Nov 22, 2020

Choose a reason for hiding this comment

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

@pmario I don't have an obvious solution that I particularly love.

However I think it is important that:
a) we try to avoid the potential confusion with attribute names if we can.
b) if we are going to proceed with this change, it needs to be a conscious decision where we consider the benefit of declaring variables in this manner vs the cost of creating usability issues and decide if this change is worthwhile.

In terms of other implementation approaches I considered requiring all user declared attributes to have a specific prefix, like var-. The problem is that while that makes it easier to identify user attributes, it makes it trickier to loop through the attributes and identify which is a widget "native" attribute since they have no specific prefix.

Every widget could have a list of attributes that get turned into variables, plus any user ones with a given prefix.

So in pseudo-code:

var widgetAttributes = ["value","name",label"];
$tw.utils.each(this.attributes,function(val,key) {
    if(widgetAttributes.indexOf(key) !== -1 || key.substring(0,4) === "var-"){
        // create a variable for this attribute.
    }
}

The other option we could consider is not to enforce a prefix for user attributes in the code, but rather make it a part of documentation and best practice. Explaining that using user attributes without a reserved prefix like "var-" could cause upgrade problems in the future. The problem with this approach is that I suspect most users will ignore it, and we will still have support issues when attribute names do clash in the future.

Does any of this perhaps inspire any ideas for you?

Copy link
Contributor

@saqimtiaz saqimtiaz Nov 22, 2020

Choose a reason for hiding this comment

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

@pmario I didn't get to see your subsequent comments while typing. But yes, a prefix for user declared attributes would solve the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sometimes using a similar approach in my plugins, to make attribute code "loopable", if there are many of them, or if I'm not sure about the names. So it's easy to change the names, because I only need to change the innitial array.

So IMO @Jermolene should have a closer look.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jermolene to try and summarize for you:

  • allowing any arbitrary name for attributes assigned by users to create variables, means that every attribute name is potentially a reserved name when/if we want to add more widget attributes in the future

  • The code in this PR reserves the $ prefix namespace for attributes. So presumably future widget attributes would have to start with $. This is a usability concern, as outside of action widgets users can reliably expect widget attributes not to start with $. Imagine if the Button widget which has been around for a long time and has many attributes, suddenly has a new attribute that starts with $.

  • One suggestion to mitigate this is, to require user assigned widget attributes to have a specific prefix, such as _ or even attr- (in which case the name of the custom attribute and the variable arising from it could be the same).

Copy link
Owner

Choose a reason for hiding this comment

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

The behaviour here is so novel compared to other widgets that I favour a big obvious prefix on the user defined attributes that are to be passed through to the action string. I think var-xxxmakes some sense for both the attribute names and variable names because it makes it clearer that this is a mini version of the <$vars> widget...

variables["attr-" + key] = val;
}
});
// "tiddler" and/or "field" parameter may be missing in the widget call. See .execute() below
variables = $tw.utils.extend(variables, {"actionValue": this.radioValue,
"attr-tiddler": this.radioTitle,
"attr-field": this.radioField
});
this.invokeActionString(this.radioActions,this,event,variables);
}
};

/*
Expand All @@ -99,6 +113,7 @@ RadioWidget.prototype.execute = function() {
this.radioValue = this.getAttribute("value");
this.radioClass = this.getAttribute("class","");
this.isDisabled = this.getAttribute("disabled","no");
this.radioActions = this.getAttribute("actions","");
// Make the child widgets
this.makeChildWidgets();
};
Expand All @@ -108,16 +123,15 @@ Selectively refreshes the widget if needed. Returns true if the widget or any of
*/
RadioWidget.prototype.refresh = function(changedTiddlers) {
var changedAttributes = this.computeAttributes();
if(changedAttributes.tiddler || changedAttributes.field || changedAttributes.index || changedAttributes.value || changedAttributes["class"] || changedAttributes.disabled) {
if($tw.utils.count(changedAttributes) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps:

if($tw.utils.count(changedAttributes) > 0 || changedTiddlers[this.radioTitle]) {
			this.refreshSelf();
			return true;
}

this.refreshSelf();
return true;
} else {
var refreshed = false;
if(changedTiddlers[this.radioTitle]) {
this.inputDomNode.checked = this.getValue() === this.radioValue;
refreshed = true;
this.refreshSelf();
return true;
}
return this.refreshChildren(changedTiddlers) || refreshed;
return this.refreshChildren(changedTiddlers);
}
};

Expand Down
1 change: 1 addition & 0 deletions core/modules/widgets/widget.js
Expand Up @@ -554,6 +554,7 @@ Widget.prototype.invokeActions = function(triggeringWidget,event) {

/*
Invoke the action widgets defined in a string
variables needs to be an object eg: {key:value, key:value}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did add 1 line comment here, since it would have saved me some time.

*/
Widget.prototype.invokeActionString = function(actions,triggeringWidget,event,variables) {
actions = actions || "";
Expand Down