FIREFLY-1327: Update job info dialog by loitly · Pull Request #1765 · Caltech-IPAC/firefly

loitly · 2025-05-12T18:05:58Z

Ticket: https://jira.ipac.caltech.edu/browse/FIREFLY-1327

Implemented as described in the ticket, with two exceptions:

Owner has been moved to the bottom section to provide a clearer separation between the top and bottom.
The default proportional font is used, so the dates are not perfectly aligned as shown in the ticket. Would you like me to switch all values to a monospaced font for better alignment? But, this will cause the dialog's font to deviate from the rest of the application.

Test: https://fireflydev.ipac.caltech.edu/firefly-1327-jobinfo-dialog/firefly/
Submit a few TAP or IRSA Catalog searches, then go to Job Monitor and click the Job Info (i-icon) to view details.

jaladh-singhal

Looks good - left minor comments in code (sorry for nitpicking)

Two probably important things from ticket that I notice missing are:

Copy icon button next to job id.
the "query" field in Parameters should be easier to find. This can be done either by moving the "query" to the top of parameters key-value pairs or by giving the query value a different font color (maybe 'primary' aka blue or 'success' aka green?) or by doing both.

jaladh-singhal · 2025-05-12T23:54:54Z

src/firefly/js/core/background/JobInfo.jsx

+    const duration =  executionDuration ? executionDuration + 's' : '';
+    const dateProps = {width: '18em', justifyContent:'space-between'};
+    return (
+        <Stack direction='row' spacing={2}>


Suggested change

<Stack direction='row' spacing={2}>

<Stack direction='row' spacing={4}>

The space-between key and value pairs is more than 16px in some cases in first column. So the horizontal spacing of second column from the first column should be more. From a quick hacking through dev-tools, 32px seems to provide a good visual grouping between the two columns.

jaladh-singhal · 2025-05-12T23:56:25Z

src/firefly/js/core/background/JobInfo.jsx

+        actualRt = Math.round((endTime - startTime) / 1000) + 's';
+    }
+    const duration =  executionDuration ? executionDuration + 's' : '';
+    const dateProps = {width: '18em', justifyContent:'space-between'};


haven't we decided to use rem throughout firefly where possible instead of em? I can only think of need of em for font sizing to maintain some proportion from parent.

jaladh-singhal · 2025-05-13T00:03:29Z

src/firefly/js/tables/ui/TableInfo.jsx

+        label = label ? label + ':' : '\u00A0';
+        value = value ? String(value) : '\u00A0';


have you tried using ' ' here instead?

I tend to avoid using raw unicode characters where possible for readability.

' ' doesn't work. It treats it as plain text.

robyww

Could you detect a select statement and do some simple formatting like we do with the ADQL panel.

You would have to parse apart the select, columns, and from. There are some utils functions buried in TAP (makeColsLines())

I think it would look better and me more skimable.

loitly · 2025-05-16T15:55:44Z

I updated the test instance with the suggested changes. Let me know what you think.
https://fireflydev.ipac.caltech.edu/firefly-1327-jobinfo-dialog/firefly/

robyww

The AQDL looks good!

jaladh-singhal

Thanks for updating it. Looks great.

Left 2 cosmetic change suggestions to make it even more polished (feel free to ignore if you don't like them). Incorporating them will make the dialog look like this (from dev tools hacking):

On a side note, the ADQL block in this dialog is not theme responsive - in dark mode, it still stays light (unlike ADQL examples in TAP ADQL panel)

jaladh-singhal · 2025-05-16T18:48:15Z

src/firefly/js/core/background/JobInfo.jsx

    const hrefs = results?.map((r) => r.href);
    const hasMoreSection = hrefs || parameters || errorSummary || aux;
    return (
        <Stack spacing={.5} p={1} sx={sx}>


Suggested change

<Stack spacing={.5} p={1} sx={sx}>

<Stack spacing={1} p={1} sx={sx}>

Looks tight, JobInfoDetails, TAPDetails, etc. already have their children with 0.5 vertical space (at least visually). To make these visual groups separate, consider using higher vertical spacing.

jaladh-singhal · 2025-05-16T18:52:14Z

src/firefly/js/core/background/JobInfo.jsx

+    return (
+        <Stack spacing={0}>
+            <KeywordBlock label='ADQL QUERY'/>
+            <PrismADQLAware text={adql} sx={{marginBlock: '-8px'}}/>


Suggested change

<PrismADQLAware text={adql} sx={{marginBlock: '-8px'}}/>

<PrismADQLAware text={adql} sx={{marginBlock: '-8px', fontSize: 'fontSize.sm'}}/>

Everyone else using sm font size in this compact panel except this Prism block

Did you mean fontSize: var(--joy-fontSize-sm)? Using fontSize.sm didn’t work for me.

Oh, I got confused. The right syntax should be fontSize: 'sm' - sx should recognize it. If not, then the var should work as you mentioned.

FIREFLY-1327: Update job info dialog

04cac1c

loitly added this to the 2025.3 milestone May 12, 2025

loitly requested review from gpdf and jaladh-singhal May 12, 2025 18:05

loitly self-assigned this May 12, 2025

loitly added the enhancement label May 12, 2025

jaladh-singhal reviewed May 13, 2025

View reviewed changes

robyww reviewed May 14, 2025

View reviewed changes

robyww approved these changes May 16, 2025

View reviewed changes

jaladh-singhal approved these changes May 16, 2025

View reviewed changes

changes from PR review

ad74c95

loitly force-pushed the FIREFLY-1327-jobinfo-dialog branch from 8027d6e to ad74c95 Compare May 16, 2025 21:11

loitly merged commit 16136a1 into dev May 16, 2025

robyww deleted the FIREFLY-1327-jobinfo-dialog branch June 5, 2025 15:39

	<Stack direction='row' spacing={2}>
	<Stack direction='row' spacing={4}>

		label = label ? label + ':' : '\u00A0';
		value = value ? String(value) : '\u00A0';

	<Stack spacing={.5} p={1} sx={sx}>
	<Stack spacing={1} p={1} sx={sx}>

	<PrismADQLAware text={adql} sx={{marginBlock: '-8px'}}/>
	<PrismADQLAware text={adql} sx={{marginBlock: '-8px', fontSize: 'fontSize.sm'}}/>

Conversation

loitly commented May 12, 2025

Uh oh!

jaladh-singhal left a comment • 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!

jaladh-singhal May 12, 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!

jaladh-singhal May 12, 2025

Choose a reason for hiding this comment

Uh oh!

jaladh-singhal May 13, 2025

Choose a reason for hiding this comment

Uh oh!

loitly May 16, 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!

robyww left a comment

Choose a reason for hiding this comment

Uh oh!

loitly commented May 16, 2025

Uh oh!

robyww left a comment

Choose a reason for hiding this comment

Uh oh!

jaladh-singhal left a comment

Choose a reason for hiding this comment

Uh oh!

jaladh-singhal May 16, 2025

Choose a reason for hiding this comment

Uh oh!

jaladh-singhal May 16, 2025

Choose a reason for hiding this comment

Uh oh!

loitly May 16, 2025

Choose a reason for hiding this comment

Uh oh!

jaladh-singhal May 16, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jaladh-singhal left a comment •

edited

Loading

jaladh-singhal May 12, 2025 •

edited

Loading

loitly May 16, 2025 •

edited

Loading

jaladh-singhal May 16, 2025 •

edited

Loading