Skip to content

Conversation

@labkey-jeckels
Copy link
Contributor

Rationale

We've accumulated a lot of controls for Panorama's QC plots. We can do a better job of arranging them and explaining them.

Changes

  • Group related controls together in three columns
  • Eliminate Apply button for date ranges
  • Do better at throwing previous requests when users rapidly update settings
  • Move some error messaging out of dialogs
  • Switch from checkboxes to radio buttons to better explain behavior

Copy link
Member

@labkey-tchad labkey-tchad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test updates look good.

@labkey-jeckels labkey-jeckels requested a review from cnathe October 8, 2025 20:42
@labkey-jeckels
Copy link
Contributor Author

@cnathe could you give the UI changes a code review?

this.lastParsedResponse = JSON.parse(response.responseText);
this.processPlotData();
// Ignore if not the most recent request
if (requestSeq !== this._qcRequestSeq)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we are calling this._qcActiveRequest.abort() before creating a new request, it seems like this case of "not the most recent request" shouldn't be hit. Is this just for the rare case where we don't abort in time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It might be overkill for this scenario, but what I saw was that abort() can still be subject to a race condition. I would have thought that JS's default threading model would make this a non-issue, but decided to do both since it wasn't much extra work

@labkey-jeckels labkey-jeckels merged commit c3b9de3 into develop Oct 10, 2025
11 checks passed
@labkey-jeckels labkey-jeckels deleted the fb_qcPlotControls branch October 10, 2025 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants