refactor: Extract helper functions and drop table-layout support #74
Conversation
1. Extract helper functions * makeLabel * makeTr * makeTd * makeHidden * makeCheckbox * makeRadio 2. Update JSDoc so type information is available to the IDE 3. Remove support for table-based layout. The required Jenkins version for the plugin is 2.332.4 since commit b06c354. Jenkins core dropped table-based layout support in 2.277.1 in favor of div-based layout.
| * @param proxy Stapler proxy object that references the CascadeChoiceParameter | ||
| */ | ||
| /* public */ function CascadeParameter(paramName, paramElement, randomName, proxy) { |
There was a problem hiding this comment.
The /* public */ interferes with JSDoc - so I've removed all occurrences of it.
| * @param paramName parameter name | ||
| * @param paramElement parameter HTML element | ||
| * @param randomName randomName given to the parameter | ||
| * @param paramName {String} parameter name |
There was a problem hiding this comment.
I've added some type info where it was easy to determine types.
| @@ -222,139 +221,6 @@ var UnoChoice = UnoChoice || ($ => { | |||
| _self.getFilterElement().setOriginalArray(originalArray); | |||
| } | |||
| } else if (parameterElement.tagName === 'DIV' || parameterElement.tagName === 'SPAN') { | |||
| if (parameterElement.children.length > 0 && parameterElement.children[0].tagName === 'TABLE') { | |||
There was a problem hiding this comment.
This is an example of code we could remove since moving the min-jenkins version past 2.277.1
| input.setAttribute("class", " "); | ||
| input.setAttribute("type", "checkbox"); | ||
| label.className = "attach-previous"; | ||
| let input = makeCheckbox(key, selectedElements.indexOf(i) >= 0, disabledElements.indexOf(i) >= 0); |
There was a problem hiding this comment.
This is an example of a helper method I've introduced where we've had multiple places where we were creating checkboxes.
| if ($(paramElement).children().length > 0 && (paramElement.children[0].tagName === 'DIV' || paramElement.children[0].tagName === 'SPAN')) { | ||
| let tbody = paramElement.children[0]; | ||
| if (paramElement.className === 'dynamic_checkbox') { |
There was a problem hiding this comment.
The code in the if block and else block were identical. I've removed the conditional logic.
There was a problem hiding this comment.
👏
From an initial look, the code looks great!
I will try to find some time to test the plug-in and review/merge it.
@rahulsom previously I used .. qunit I think, to write unit tests. But that was a bit clumsy and not reliable nor easy to maintain the tests. Do you think we could/should use unit tests for this new code, especially now that it's more modular? Maybe with jest, mocha, chai, etc?
Thanks!
I have very little experience with JS code. I've written unit tests for React apps, but not much else. If you have a reference in mind, I'm glad to add that. |
|
Ah, I thought you'd have a preferred solution. I'm used to these three I mentioned before, and have also used Karma in the past with angularjs. I will see how complicated it would be to add something like that when I review this PR (it could be that it makes more sense to merge and then open another issue for that to avoid blocking your work) 👍 Thanks heaps for preparing the PR. |
|
😅 I've got some tests working for the javascript code. It uses jest. The code had to be split up into the tested code in I'll create a separate PR with that. |
rahulsom
force-pushed
the
extract-reusable-helpers
branch
from
8a7023e to
c94bf7a
Compare
|
(starting from this one as you've smartly built the other PR's on top of each change 🙂) |
Testing done
No functionality has changed. All unit and acceptance tests pass.
Submitter checklist