IRSA-7274: Download Dialog & Checkbox improvements #1855

kpuriIpac · 2025-10-01T21:42:29Z

Ticket: https://jira.ipac.caltech.edu/browse/IRSA-7274

When a user selects cutout from the download dialog, we now notify them of the current cutout size (and point them to the cutout dialog if they need to change it)
Made some improvements to CheckboxGroupInputField that allow All Data to basically operate as a Select All button
- this logic does exist elsewhere like for checkboxes in tables, but that doesn't use CheckboxGroupInputField
- For CheckboxGroupInputField, this logic was partially in place (e.g. it does expect _all_ and _none_ as options).
  - Now with the added logic, it allows you to easily assign a Select all type option, which when selected, selects all other options, and is automatically unselected when you unselect any of the other options.
  - this logic is now generic and can be applied anywhere CheckboxGroupInputField is used

Testing:

Euclid & Spherex: Do a search, select a few rows, open the download dialog
- play around with the All Data selection (select/unselect it, manually select all other options when All Data is unselected, etc.)
- When All Data is selected, or when you manually select Cutout, you should see the cutout size show up dynamically
Firefly: Mostly regression testing, test any piece of UI that uses CheckboxGroupInputField, a couple of instances are:
- in the Plot/Chart options under X label or Y label like grid, reverse, top, log (select/unselect any of these and apply to make sure it works as expected)
- The checkboxes under Select Data Set on the Images tab.
  - As an example, try selecting the Mission: Wise and then Wise ALLWISE Atlas (the first option). Make sure all of the nested options under it are also selected.
    - play around with the checkboxes to ensure they work as expected.

loitly

I’m approving this PR since it works as expected. However, I’m not sold on the implementation or the presentation of the “All Data” checkbox. I think it would be good for someone else to review that part as well.

loitly · 2025-10-01T23:34:46Z

src/firefly/js/templates/common/ttFeatureWatchers.js

+        //ensure '_all_' ("All data") is always included, even as a fallback when semList is empty
+        //and make it the first option 
+        options.unshift({ label: 'All Data', value: '_all_' });


I don’t like the idea of presenting an “All data” checkbox that looks identical to the other checkboxes. Typically, a “Select All” control isn’t presented in the same way as the rest of the checkboxes.

I also don’t like the weak coupling of _all_ as a reserved value of the CheckboxGroup.

Yes you're right. I tried separating the All Data out onto a separate line and keeping the other options nested under it, but I ran into some weird bugs with separating out the checkbox groups that way. Let me give it a try again, though.

The other option is removing All Data as an option for now. What I don't like is how it works now (before this PR), where when you can select All Data and, say, 2 other options (out of 6 total options). But since you have All Data selected, it's anyway going to download all the data (even though you only have 2 other options selected).

So in my opinion, we either fix All Data to work like a Select all checkbox or we remove it altogether as an option (which is logic that we do have elsewhere on firefly as well).

If you need a reusable Select All widget, you can export one from CheckboxGroupInputField. It should only take fieldKey and the optional props groupKey and label. That’s all the information required to fully control the checkbox group. You can then render it anywhere you like. An option for your case is next to the Products to Download label.

kpuriIpac · 2025-10-03T02:55:41Z

@loitly new changes are ready for review (tomorrow or Monday when you get a chance). A summary of changes:

I added a SelectAllCheckbox component.
Modified existing logic in handleOnChange to deal with turning on/off the Select all checkbox and other checkboxes
- as we discussed, the changes to your selections persist so even when you switch rows the download dialog selections remain the same
While this still makes use of _all_, an example of generic usage of SelectAllCheckbox is how I've implemented it in ttFeatureWatchers
- one call to SelectAllCheckbox for the Select All checkbox (customizable name) and one call to CheckboxGroupInputField directly for the rest of the options, both using the same fieldKey
So now any usage of CheckboxGroupInputField can be coupled with a Select all type checkbox, but for other existing uses of CheckboxGroupInputField where it's called without one, everything should still work the same.

Let me know what you think. And if it looks good then, should I document some of this usage in the code as comments above SelectAllCheckbox?

BTW: testing instructions are same as above. I just changed All Data text to Select all instead. And there's a spacing issue in the dialog (awkward/too much space), I'll fix that in a small commit next.

Also, right now the Select all checkbox shows up next to the label Products to Download. I can try it on the next line as well.

loitly

Please keep the 'Select All' logic confined to SelectAllCheckbox component.

loitly · 2025-10-03T17:39:49Z

src/firefly/js/templates/common/ttFeatureWatchers.js

+    return !isDatalink && (
+        <Stack spacing={2}>


Minor: I thought you show Products to Download when it is a datalink table.
Also, since isDatalink is static, does it make sense to reserve a prop for it? Logic could be

isDatalink && <ProductTypesBlock/>

Yes that's intended to be !isDatalink. But I can see why that's confusing based on our conversation yesterday. We want to show Products to Download for rows pointing to datalink (so rows containing an access_url). For example when you do an Image search on Euclid. So by looking at the datalink we can get a list of the products available to download, and then we display them to the user.

But when you actually have a datalink table directly, like so:

(you get to this by extracting the current row and doing Show table of all Euclid products)

Since this table is directly datalink, and users can see the semantics directly (and download each one), there's no point of showing the Products to Download here.

I agree about the prop logic, it's unnecessary there. I'll change to:

!isDatalink && <ProductTypesBlock/>

Oh and BTW I notice the bug there. Generate Download Script & Prepare Download both showing up for the datalink table. Unfortunately on ops. I had fixed this bug, but after having made some more changes (and fixing a spherex bug related to this), it must have creeped back in. Creating a bug ticket for myself.

loitly · 2025-10-03T17:41:16Z

src/firefly/js/templates/common/ttFeatureWatchers.js

+                <Stack spacing={1} m={1}>
+                    <CheckboxGroupInputField
+                        fieldKey='productTypes'
+                        options={dynamicOptions.filter((op) => op.value !== '_all_')}


There should not be any reference to _all_.

loitly · 2025-10-03T17:44:11Z

src/firefly/js/templates/common/ttFeatureWatchers.js

+                <FieldGroup groupKey={tbl_id}>
+                    <Stack>
+                        <DownloadButton key={keysToWatch}
+                            buttonText = {downloadType === 'script' ? 'Generate Download Script' : 'Prepare Download'}>
+                            <DownloadOptionPanel {...{


Minor: it's a good idea to move this into a separate component. It makes it easier to read as well as identifying props(input).

Not sure about this one, let's talk briefly on Monday morning before the meeting?

loitly · 2025-10-03T17:47:31Z

src/firefly/js/ui/CheckboxGroupInputField.jsx

 function handleOnChange(ev, viewProps, fireValueChange) {
-    // when a checkbox is checked or unchecked
-    // the array, representing the value of the group,
-    // needs to be updated
-    const value= convertValue(viewProps.value,viewProps.options);
-
    const val = ev.target.value;
    const checked = ev.target.checked;
-    const curValueArr = getCurrentValueArr(value);
-    const idx = curValueArr.indexOf(val);
-    if (checked) {
-        if (idx < 0) curValueArr.push(val); // add val to the array
+    const allValues = (viewProps.options ?? []).map((option) => option.value);
+
+    //expand '_all_' into the full options list and use that as the list of curr values array
+    const normalizedValue = convertValue(viewProps.value, viewProps.options);
+    const curValueArr = getCurrentValueArr(normalizedValue);


Logic should not be here. It should be in the SelectAllCheckbox component.

Ok yes I understand this now. Separation of concern as well! I'll implement this.

kpuriIpac · 2025-10-03T23:16:38Z

Thanks for the feedback again @loitly. This revision should be a lot cleaner:

Separated all SelectAllCheckbox logic from pre-existing CheckboxGroup logic.
- and added a JSDoc comment for SelectAllCheckbox
no reference to _all_ anymore outside of SelectAllCheckbox logic (e.g. in ttFeatureWatchers even when creating the dynamicOptions list, not injecting _all_ into the list anymore)
- actually, in the latest commit I made a bug fix which also included getting rid of _all_ altogether. Not needed in SelectAllCheckbox.
all 3 test builds updated

I'll work on fixing the styling (bit too much space between the product types and the flattened file structure option, even more when you see the cutout size line).

Let me know what you think?

loitly

Tested the changes and it works as expected.
Okay to merge when ready.

loitly · 2025-10-06T15:29:11Z

src/firefly/js/templates/common/ttFeatureWatchers.js

+    const isCutoutSelected = (() => {
+        if (!productTypes) return false;
+        const vals = productTypes.split(',').map((val) => val.trim());
+        return vals.includes('#cutout');
+    })();


Out of curiosity, is there a reason this is defined as a function and then called right away? Isn't this the same as ...

const isCutoutSelected = productTypes?.split(',').map((val) => val.trim()).includes('#cutout');

Yes,I was doing some more stuff in it in previous iterations (like checking for #all in addition to #cutout) - but it's now simple enough to have it be the one line variable instead of function like you suggested. So I'll make the change.

loitly · 2025-10-06T16:01:54Z

src/firefly/js/ui/CheckboxGroupInputField.jsx

+    return (
+        <Stack direction='row' alignItems='center'>
+            <Checkbox
+                size='sm'


The code looks good. Thanks for the changes.
If you want to make it more reusable, consider removing the root Stack component and adding a props parameter. Then, spread those props into the Checkbox. This will allow users of the component to customize its styling more easily.

kpuriIpac · 2025-10-10T00:16:48Z

@loitly I only made minor changes, but if you want to take a look at the latest commit, here's what it essentially does:

minor cleanup based on your last suggestion
get rid of the All data or Select all checkbox when there are no product types to display
- for example, in DCE (https://irsa-7274-cutouts-downloads-imrpove.irsakubedev.ipac.caltech.edu/irsaviewer/dce) you would see this before:
- which obviously does not make sense as there are no options, so in case options is empty now I don't display this checkbox anymore

kpuriIpac · 2025-10-10T00:17:08Z

@robyww the bug I found in firefly is that if we initialize PrepareDownload for a table for which there's no MetaConst.DATA_SERVICE_ID for a table, then the viewerId in ttFeatureWatchers will be undefined.

In this case, we default to the default dataProductsComponentKey in the Cutout.js whose value is DataProduct.defaultComponentDataKey
but i think the actual default dataProductsComponentKey that we use is the same as the dataTypeViewerId or dpId which happens to be DataProductsType...traced this all the way back to line ~17 in MetaDataMultiProductViewer
- So, because of these two values not being the same, when we update cutouts in firefly (you can try IVOA CADC search here: https://fireflydev.ipac.caltech.edu/irsa-7274-cutouts-downloads-imrpove/firefly), you'll see the cutout updated in the UI. But when you Prepare Download and select Cutout, you won't see the updated cutout value.
The fix: changing the dataProductsComponentKey in cutout.js to this value (DataProductsType) will resolve this bug, but I wanted to check with you if this makes sense.

If this is too verbose, we can discuss briefly tomorrow morning on zoom. I'd like to just put this fix into the PR as well before merging then.

… cutout

kpuriIpac added this to the 2025.5 milestone Oct 1, 2025

kpuriIpac requested review from loitly and robyww October 1, 2025 21:42

kpuriIpac self-assigned this Oct 1, 2025

kpuriIpac added the enhancement label Oct 1, 2025

loitly approved these changes Oct 1, 2025

View reviewed changes

kpuriIpac force-pushed the IRSA-7274-cutouts-downloads-imrpove branch from e89f3a7 to 7f579fb Compare October 3, 2025 00:21

loitly requested changes Oct 3, 2025

View reviewed changes

kpuriIpac force-pushed the IRSA-7274-cutouts-downloads-imrpove branch 3 times, most recently from b7e24b5 to b398e87 Compare October 3, 2025 23:11

kpuriIpac force-pushed the IRSA-7274-cutouts-downloads-imrpove branch from b398e87 to 598f00f Compare October 3, 2025 23:26

loitly approved these changes Oct 6, 2025

View reviewed changes

kpuriIpac force-pushed the IRSA-7274-cutouts-downloads-imrpove branch from e430668 to 7a69fc1 Compare October 9, 2025 23:57

kpuriIpac force-pushed the IRSA-7274-cutouts-downloads-imrpove branch from 7a69fc1 to a7b11c2 Compare October 10, 2025 21:04

IRSA-7274: display cutout size in download dialog and bug fix firefly…

57ccaa9

… cutout

kpuriIpac force-pushed the IRSA-7274-cutouts-downloads-imrpove branch from a7b11c2 to 57ccaa9 Compare October 10, 2025 21:46

kpuriIpac merged commit 1ca80b1 into dev Oct 10, 2025

kpuriIpac deleted the IRSA-7274-cutouts-downloads-imrpove branch October 10, 2025 21:47

IRSA-7274: Download Dialog & Checkbox improvements #1855

IRSA-7274: Download Dialog & Checkbox improvements #1855

Uh oh!

Conversation

kpuriIpac commented Oct 1, 2025

Uh oh!

loitly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpuriIpac Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpuriIpac commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loitly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpuriIpac commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loitly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpuriIpac commented Oct 10, 2025

Uh oh!

kpuriIpac commented Oct 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kpuriIpac Oct 1, 2025 •

edited

Loading

kpuriIpac commented Oct 3, 2025 •

edited

Loading

kpuriIpac commented Oct 3, 2025 •

edited

Loading