Add remembering filter properties of Access Policy Templates grid in browser url #16084
Conversation
manager/assets/modext/widgets/security/modx.grid.access.policy.template.js
Show resolved
Hide resolved
| ,search: function(tf,newValue,oldValue) { | ||
| var nv = newValue || tf; | ||
| this.getStore().baseParams.query = Ext.isEmpty(nv) || Ext.isObject(nv) ? '' : nv; | ||
| ,searchPolicyTemplate: function(cb, newValue, oldValue) { |
There was a problem hiding this comment.
Why you have oldValue in the signature if it's not in use?
There was a problem hiding this comment.
It may be used in the future, let it be (it was originally indicated).
There was a problem hiding this comment.
FWIW I don't have a strong position on this although I'd moreso side with only including the params you're using in cases like this—where the listener callback is calling a secondary function (such as has been done here). I do think with the callbacks themselves, the entire signature should always be provided to make it easier for devs to know what they can do within the function without having to necessarily go to the listener's documentation.
There was a problem hiding this comment.
I actually have a strong opinion on this and there should be no unused variables or parameters. If it will be required in the future, let's add it in the future.
There was a problem hiding this comment.
Well, I did not add them, so I see no reason to remove them. Again, this is true for most other grids.
If you do not agree and you have the time and desire, you can, after all the corrections in the PRs, go through the files and remove unnecessary variables.
There was a problem hiding this comment.
wdym you didn't add them? You are adding searchPolicyTemplate function with clearly unused oldValue
There was a problem hiding this comment.
Well, not really, I just bring the code to the form as in the rest of the grids, at least I try. Those we can leave it all with the old search code - everything will work same.
What does it do?
Why is it needed?
Allow to copy/paste the state of the filter with the browser url.
Related issue(s)/PR(s)
#16081
#16063
#15946
#15942
#15935
#15186
#15185
#15184
#15183
#15182
#15181
#15115
#14086