Fix zone deletion. #30
Conversation
a4d5ddb to
e8a469f
Compare
| // The id is of the form "zone-<index>". We extract the index by | ||
| // slicing off the first five letters and "casting" to integer. | ||
| index = +id.slice(5), | ||
| i; |
There was a problem hiding this comment.
This works but seems quite fragile (not your fault, just how the code already was). I'd prefer something like: In the add method, before appending the new DOM node (zoneTemplate(zoneObj)), you can use .data('index', num) to save the index to the node as a JavaScript integer. Then to read the index here, it's just $el.data('index') with no casting or string processing needed.
Eventually, this code should be cleaned up a bit - data is stored redundantly in the DOM and in JavaScript, and there are several ways of identifying each zone (id, index, i, ...). It's quite confusing to read.
There was a problem hiding this comment.
@smarnach @bradenmacdonald also, is I'm the only one that find declaring a loop variable at the top of the function a strange practice? I.e. instead of for (var i=...) do var i; for (i=...?
There was a problem hiding this comment.
@bradenmacdonald @e-kolpakov The code will be fragile anyway, and will break if you add a new class to the div. But good point anyway, we can at least make the code we add less fragile, even though it doesn't help right now.
Regarding the loop variable, I intentionally declared it at the top since it is used after the loop as well. I know it would be scoped to the function in any case, but coming from C++, I feel this way gets across the intention better -- it's not limited to the loop. Also, https://github.com/edx/edx-platform/wiki/Javascript-standards-for-the-edx-platform#good-practices (though it doesn't mention loop variables explicitly).
|
There was a build error on Travis CI so I ran the build again for you and it passed. That means it was just a flaky test, and I'm pretty sure it's unrelated to this PR. I tested this in Studio and it works for me, and the code is no worse than what's there already so 👍 That said, I gave one comment for improvement if time permits. The JS here kinda gives me a headache and I'd love to see it cleaned up and refactored even more but I know there's not time for that now. |
| id = classes.slice(classes.indexOf('zone-row') + 9), | ||
| // The id is of the form "zone-<index>". We extract the index by | ||
| // slicing off the first five letters and "casting" to integer. | ||
| index = +id.slice(5), |
There was a problem hiding this comment.
@smarnach this will break if any other class added after 'zone-' (e.g. zone-row zone-2 zone-disabled). +1 for @bradenmacdonald 's suggestion below
There was a problem hiding this comment.
@e-kolpakov As noted above, the original code will break in this case anyway. The code needs some rather fundamental refactoring to make it sane. Braden's suggestion is good anyway, and I implemented it.
When deleting a zone, the old version only removed it from the DOM, but not from the underlying data structure that is sent to the server when the save button is pressed. This resulted in zones not actually being deleted. This commit fixes this issue. Also fix a typo in a variable name while we are here.
e8a469f to
8869d0c
Compare
|
@bradenmacdonald @e-kolpakov Thanks for the review. I agree the code could do with some refactoring (actually, with quite a bit of it); it is fragile and has a rather questionable choice of data structures. I didn't have time to do this yesterday since I had to learn a lot of things along the way, so I guess this has to wait for some later time. |
When deleting a zone, the old version only removed it from the DOM, but not from the underlying data structure that is sent to the server when the save button is pressed. This resulted in zones not actually being deleted. This commit fixes this issue. Also fix a typo in a variable name while we are here.
I have tested this change in a local devstack (which caused more trouble than expected), and it is working fine. There are some bits of the JS code that aren't quite clear to me -- e.g. the difference between _fn.build.form.zone.count and _fn.build.form.zone.formCount. Given that the deadline for this issue is tomorrow, I decided against trying to clean up the code any further than necessary.