Refactor web extension #407
Conversation
There was a problem hiding this comment.
Very minor comments on the code itself.
Only question: Do we have any tests for the plugin? If not, why not? If yes, please add some to, at minimum, verify the code fixes the bugs and ensure the refactor doesn't break existing behavior or introduce new bugs.
The code changes appear to be a very nice simplification. But I am always hesitant to refactor without any way to validate.
| <br/> | ||
|
|
||
| <label>Tool Name:</label> | ||
| <input id="toolName"/> | ||
| <input id="tool"/> |
There was a problem hiding this comment.
Is there a reason this differs from the javascript package naming conventions?
There was a problem hiding this comment.
I changed the html id's to match the data tag names defined here:
Originally I intended to add a layer of abstraction that defines the tag names and js property names for each setting and could be imported into getInitialSettings.js and options.js. But I wanted to limit the scope of this PR to the web extension.
| <br/> | ||
|
|
||
| <label>Tool Version:</label> | ||
| <input id="toolVersion"/> | ||
| <input id="version"/> |
| <br/> | ||
|
|
||
| <label>Tool Name:</label> | ||
| <input id="toolName"/> | ||
| <input id="tool"/> |
There was a problem hiding this comment.
Same as previous comment.
No plug-in tests exist. I think the plug-in has historically been an experimental tool only. That said it's already in my notes to write some plug-in tests. |
|
Alright. I'm hesitantly approving since this is a blocker. |
This PR refactors the web extension, most of the changes simply reduce repeated code but there are a few actual changes.