FIREFLY-2021: Improve job monitor UI - #1986
Conversation
jaladh-singhal
left a comment
There was a problem hiding this comment.
@loitly this is working as expected. Clear filter looks correct as in other tables.
However I noticed a bug: Apply any filter to enum type columns -> click on clear filter button -> the enum selection (▼) disappears. Sometimes they come back after a delay though (can't reliably reproduce the delay duration)
| <Stack sx = {{[`& .${badgeClasses.root}`]: {margin: '1px 3px 0 0'}}}> | ||
| <ClearFilterButton iconButtonSize='34px' onClick={clearFilters} | ||
| <Stack sx={{[`& .${badgeClasses.root}`]: {margin: '1px 3px 0 0'}}} {...props}> | ||
| <ClearFilterButton iconButtonSize={iconButtonSize} onClick={clearFilters} |
There was a problem hiding this comment.
I'd rather pass props to ClearFilterButton and maybe a slotProp.root to the Stack - this way you can override tip too. I don't think the default tooltip makes sense for job monitor but @lrebull can verify the wording.
| return ( | ||
| <Stack alignItems='center' justifyContent='space-between' height={1} mt='1px'> | ||
| <HeaderText val={col?.label || col?.name} level='title-sm'/> | ||
| <OptionsFilterStats tbl_id={jobHistoryTblId} alignSelf='start'/> |
There was a problem hiding this comment.
Another placement of this button could be:
2. in the job summary area below/right of time zone
3. bring the table toolbar back and let it show filter controls how we do elsewhere?
I'm honestly not sure which one is better since this is not a typical table.
There was a problem hiding this comment.
I don't like the center align too much because it seems awkward to have clear filter there.
Another option:
Move the entire ‘Job Summary’ row to the Table’s header and enable it with filter. It’s a bigger change, but worth doing. I will give it a try, but will require scientist to approve new look.
- Preserve sort and filter state across automatic refreshes - Add a clear filter button when filters are active - Show a loading indicator while data is being refreshed - Highlight newly added job after sort/filter is applied instead of always placing it at the top
…lter/clear-filter
a6e81bb to
3cdc055
Compare
jaladh-singhal
left a comment
There was a problem hiding this comment.
@loitly I like your updated layout utilizing table toolbar, also the conditional notification part looks clean and separate now.
I've left some stylistic/CSS improvement comments - none of them are dealbreaker for merging this as-is.
| onFilterSelected, sx}) { | ||
| return ( | ||
| <Stack alignItems='center' height={1} justifyContent='space-between' py='2px' sx={sx}> | ||
| <Stack alignItems='center' height={1} justifyContent='space-between' pt='4px' pb='2px' sx={sx}> |
There was a problem hiding this comment.
| <Stack alignItems='center' height={1} justifyContent='space-between' pt='4px' pb='2px' sx={sx}> | |
| <Stack alignItems='center' height={1} justifyContent='space-between' pt={0.5} pb={0.25} sx={sx}> |
| * total height up front, so these must match what HeaderCell renders. HeaderText's font size is | ||
| * pinned to headerFontSize so the text rows stay in sync; headers do not scale with the font . | ||
| */ | ||
| export const headerFontSize = 12.25; // Joy's fontSize.sm at firefly's root size |
There was a problem hiding this comment.
I wonder if we can pull it from joy ui theme instead of hardcoding this. What happens if Rubin or someone else configures Firefly to render different than our default font at a different size?
| label: 22, | ||
| units: headerLineHeight, | ||
| types: headerLineHeight, | ||
| filter: 25, |
There was a problem hiding this comment.
same here for label and filter.
| sx={{'& .fixedDataTableCellGroupLayout_cellGroup > :last-child': {borderRight: 'none'}, ...sx}} | ||
| showToolbar={true} | ||
| showTitle={false} | ||
| leftButtons={[() => <JobSummary jobs={jobs} overflow={overflow}/>]} |
There was a problem hiding this comment.
[No fix required] Just noticed that JobSummary doesn't wrap in smaller than default width of this table. Although this is how ops behaves too.
@loitly not sure if you noticed this issue - it happens intermittently for me. |


Ticket https://jira.ipac.caltech.edu/browse/FIREFLY-2021
Test:
https://firefly-2021-job-monitor-update.irsakubedev.ipac.caltech.edu/applications/spherex/
https://firefly-2021-job-monitor-update.irsakubedev.ipac.caltech.edu/firefly/
Addressed the issues reported in the ticket:
Additional Changes after PR:
Test URL instances updated with the latest change. Please test again.