Form customization front end fixes/improvements #15836
Conversation
|
REVIEWERS: Note that the unit test for this (modActionDomTest.php) will need to be changed for this PR to pass. I'm just not sure who should do that. Basically, the parameters given that are formatted as an array need to be changed to a string. Please advise. |
|
@smg6511 Please correct errors for passing tests |
|
@Ibochkarev OK, the unit test is fixed. |
|
@theboxer - Just a general note as I work on your feedback: The methods I updated were primarily done so with the main purpose in mind (changing the unnecessary array-formatted params to string params), as well as renaming params so they're immediately understandable and providing doc blocks for the methods I worked with. I did not really alter them too much otherwise. As you're pointing out, maybe some re-writing could be done, but that's not what I was looking to address with this PR. I've found that a lot of reworking needs to be done overall with the element creation and form customization UIs that may not be immediately apparent. See my PR #15773 ... it'll give you a pretty good idea of what I'm referring to. These type of changes are too big to be worthwhile for 2.x and are targeted for 3.x. |
|
@smg6511 Sorry, I kind of missed that this is not a PR for 3.x, priorly knowing that this is for 2.x I wouldn't care about the complexity :)) Feel free to ignore them all. I'd love to see the overall code quality to improve for 3.x, that's why I was trying to make these suggestions - as it makes the code more readable and easier to follow. |
|
@theboxer - No problem. I'm totally with you on upping the code quality! As I was playing with this I discovered how fragile the form customization is; once you have a conflicting rule (whether in one profile or across multiple ones) unexpected behavior creeps in. That'll be a bigger task for a different day ;-) |
|
@theboxer - There's still a change requested flagged, but I can't see where it is. Not sure if I dismiss that (and if so, how) or you do... |
| * @param {String} targetId - Text id of the TV's destination (region/tab) | ||
| * @return {void} | ||
| */ | ||
| ,moveTV: function(tvId, targetId) { |
There was a problem hiding this comment.
I'm afraid that this change can't go to 2.x as it technically breaks backwards compatibility (changing the array arg to string).
There was a problem hiding this comment.
Can you tell me how and at what version this would break it? Same with the others. I'd like to try to fix this issue for 2.x if possible, but it'll likely be unfixable if the FC rules were somehow saved in a serialized way at some point. Thx ;-)
There was a problem hiding this comment.
Well you changed the signature of the function from accepting array/string and string, to accept string and string. I know you changed all the usages within modx, but if there's an extra out there that's using the array & string signature it'll break. Which is not allowed when following semver.
There was a problem hiding this comment.
Let me ask you this: Would there be any reason for an extra to interact directly with the FC rules/logic? I might be short-sited on this, but I can't imagine an extra utilizing these methods in any way.
On the breaking change/semver point, I get that and am wholeheartedly for following common and strict protocols. For the sake of debate, is it worth not fixing a bug to maintain protocol when a change technically breaks the protocol but has a slim-to-none practical chance of actually breaking functionality? Arguably, the particular issue this PR addresses is low priority as far as bugs go, but addressing it did lead to discovering some seemingly unnecessarily complicated code.
Anyway, regardless of whether we drop this PR for 2.x, should I assume that the BC we're debating will not be an issue if submitted for 3.x?
There was a problem hiding this comment.
Is there a specific reason this bug could not be resolved without changing the function signatures?
There was a problem hiding this comment.
Yes, the issue is that right now FC can't handle a comma in a field label because the current code reads that as a separator. I'm not sure there's a way around it without changing the signature.
| } else { | ||
| cto.label.update(vals[0]); | ||
| container.label.update(newLabel); |
There was a problem hiding this comment.
Same as the previous, breaks backwards compatibility by changing array to string.
| * @param {String} fieldId - Text id or name of field whose label is being hidden | ||
| * @return {void} | ||
| */ | ||
| ,hideField: function(fieldId) { |
| * @param {String} newLabel - The replacement label text | ||
| * @return {void} | ||
| */ | ||
| ,setLabel: function(fieldId, newLabel){ |
Ibochkarev
removed
the
pr/review-needed
|
Closing this for 2.x. A port to 3.x has been merged. |
* Port changes from PR #15836 * Update modActionDom.php * Remove commented code * grunt build Co-authored-by: Jason Coward <jason@opengeek.com>
What does it do?
Reworks the methods responsible for moving and changing field/tab visibility and labelling. Code optimizations were made and method documentation was added.
Why is it needed?
Most of these methods were written to receive/process serialized/array data, but there appears to be no reason (any longer at least) to keep them that way—especially given that problems have arisen such as the one referenced by this PR. The changes allow for the use of commas in labels and provides simplification and clarification of the relevant code.
How to test
Clear your caches and remember to run
grunt buildwithin the _build/templates/default directory to compile the changes into the main js file. Create one or two test TVs, then create a Form Customization profile with at least one rule. Within your rule(s), experiment with re-labelling and hiding various resource fields along with your test TVs. Also create a new tab in Regions tab and verify that assigning your TVs to that tab (or the standard regions) works as expected.Related issue(s)/PR(s)
Resolves #15357.