Move TargetedMS QC/chart UI off ExtJS to plain JS + HTML - #1257
Move TargetedMS QC/chart UI off ExtJS to plain JS + HTML#1257ankurjuneja wants to merge 5 commits into
Conversation
| public void addClientDependencies(ClientDependencies dependencies) | ||
| { | ||
| dependencies.add("Ext4"); | ||
| dependencies.add("Ext4"); // LABKEY.vis (vis/vis) still uses Ext.isArray for log-scale axes |
There was a problem hiding this comment.
@ankurjuneja this is the PR I was thinking of when I tagged you for the changes here LabKey/platform#7865. Not sure if that changes this dependency at all, but wanted to call it out.
There was a problem hiding this comment.
Thanks @cnathe, I will verify (and remove) if we need the Ext4 dependency for calibration curve.
There was a problem hiding this comment.
Reminder to check on this.
| * Licensed under the Apache License, Version 2.0: http://www.apache.org/licenses/LICENSE-2.0 | ||
| */ | ||
|
|
||
| Ext4.define('Panorama.Window.AddTraceMetricWindow', { |
There was a problem hiding this comment.
ConfigureMetricsUIPage still has Ext locators.
| const title = op === 'insert' ? 'Add New Trace Metric' : 'Edit Trace Metric'; | ||
|
|
||
| // In update mode, pick the mode based on the stored values; default to the time-value mode. | ||
| const isTraceValueMode = op === 'update' && metric.TraceValue > 0; |
There was a problem hiding this comment.
The check on line 79 allows zeros when saving, but this treats them as a sentinel. Probably best to make line 79 ensure a positive value, which matches the old validation.
| return String(a.TextId).localeCompare(String(b.TextId)); | ||
| }); | ||
| let html = '<option value="">-- Select trace --</option>'; | ||
| traces.forEach(function(row) { |
There was a problem hiding this comment.
Low priority, but if the previously saved value is no longer present, it won't be in the list. The old code always added the saved value. We could do the same here.
| const metricName = $('#lk-trace-metric-name').val().trim(); | ||
| checkMetricNameExists(metricName, function(exists) { | ||
| if (exists) { | ||
| showError('A metric with the name "' + LABKEY.Utils.encodeHtml(metricName) + '" already exists. Please choose a different name.'); |
There was a problem hiding this comment.
showError uses .text() so this will be double-encoded. Remove the encoding here.
| const rounded = Math.round(value * 100000) / 100000; | ||
| // Use scientific notation if the number is large or very small | ||
| if (exp && (rounded > 10000 || rounded < -10000 || (rounded > -0.00001 && rounded < 0.00001))) | ||
| rounded.toExponential(4) |
There was a problem hiding this comment.
| rounded.toExponential(4) | |
| return rounded.toExponential(4) |
| </tr> | ||
| <tr> | ||
| <td class="sc-label"><label for="sc-width">Width</label></td> | ||
| <td><input type="number" id="sc-width" value="<%= bean.getInitialWidth() %>"></td> |
There was a problem hiding this comment.
| <td><input type="number" id="sc-width" value="<%= bean.getInitialWidth() %>"></td> | |
| <td><input type="number" id="sc-width" value="<%= bean.getInitialWidth() %>" min="1" step="1"></td> |
| areaElement.style.width = parseInt(widthInput.value) + 'px'; | ||
| areaElement.style.height = parseInt(heightInput.value) + 'px'; | ||
| timeElement.style.width = parseInt(widthInput.value) + 'px'; | ||
| timeElement.style.height = parseInt(heightInput.value) + 'px'; |
There was a problem hiding this comment.
Ideally we'd handle a NaN as type="number" isn't particularly strict.
| this.queryError.setVisible(true); | ||
| } | ||
| }); | ||
| LABKEY.Query.saveRows({ |
There was a problem hiding this comment.
In my local testing, this can be sluggish to save. There's no feedback for the user while it's working, which led me to click on Save a second time, which then produced a key violation. This change presumably hasn't impacted the perf, but it would be nice to show a spinner and disable the buttons to avoid a double-submit.
| public void addClientDependencies(ClientDependencies dependencies) | ||
| { | ||
| dependencies.add("Ext4"); | ||
| dependencies.add("Ext4"); // LABKEY.vis (vis/vis) still uses Ext.isArray for log-scale axes |
There was a problem hiding this comment.
Reminder to check on this.
| public void addClientDependencies(ClientDependencies dependencies) | ||
| { | ||
| dependencies.add("Ext4"); | ||
| dependencies.add("Ext4"); // still needed by QCMetricConfigLoader.js |
There was a problem hiding this comment.
Looks like it's just for JSON parsing. Can we migrate that?
3 participants
Rationale
We are gradually removing our dependency on ExtJS. ExtJS is old, heavy, and something we want to stop using. Each of these screens used ExtJS only as a wrapper around plain data or existing non-ExtJS chart code, so we can swap in simple HTML, plain JavaScript, and jQuery without changing what the user sees. This makes the code lighter, easier to read, and easier to maintain.
Related Pull Requests
Changes