Firefly-1451: Embedded Position Search Panel Enhancements by kpuriIpac · Pull Request #1548 · Caltech-IPAC/firefly

kpuriIpac · 2024-04-26T22:16:28Z

see ticket details
made header as small as possible when search panel is open, and when collapsed, display a search summary.
reduced opacity of search panel on mouse click anywhere outside the search panel, but hover/mouseover the search panel brings it back to 100% opacity
corresponding irsa-ife PR: https://github.com/IPAC-SW/irsa-ife/pull/323

Testing:

Euclid: https://firefly-1451-embed-panel-enhanced.irsakudev.ipac.caltech.edu/applications/euclid
- Play around with the searches, collapse the panel for cone and multi-object (before and uploading a file - should work in both cases).
DCE: https://firefly-1451-embed-panel-enhanced.irsakudev.ipac.caltech.edu/irsaviewer/dce
- the search panel will be a bit buggy for DCE right now when collapsed, since there's no groupKey being sent to EmbeddedPositionSearchPanel
- but if we wrap the call to EmbeddedPositionSearchPanel in a FieldGroup with a groupKey, then this may work as well (or if we decide not to make it collapsible for DCE, then the collapse behavior can be made conditional in EmbeddedPositionSearchPanel).

loitly · 2024-04-29T18:35:42Z

    header: oneOfType([string, func, element]),
+    expandedHeader: oneOfType([string, func, element]),


Instead of adding new prop and slotProps, I suggest fixing the header prop so that it does what it was designed to do. header should be:

header: oneOfType([node, func]), // func is called with isOpen as a parameter

robyww

This is looking good. The only thing I see now is making the header small when the panel is opened.

kpuriIpac · 2024-05-08T20:41:49Z

+                <CollapsibleGroup
+                    sx={{borderWidth: '0px',
+                        '& .MuiAccordionSummary-root': {
+                            paddingBlockStart: '0.5rem',
+                            paddingBlockEnd: '0.5rem',
+                            minBlockSize: '1rem',
+                            '&.Mui-expanded': {
+                                height: '1rem '
+                            }
+                    }}}>


@loitly do you mind reviewing this latest commit? it controls the styling for the header when the search panel is open (the idea was to make it as small as possible, as the scientists don't want the search panel to take up too much space) but still keep it collapsible.

Feel free to review other stuff too, but if this looks ok I can go ahead and merge this PR. Here's the test build: https://firefly-1451-embed-panel-enhanced.irsakudev.ipac.caltech.edu/applications/euclid

loitly

Bug: Clicking around while in collapsed mode did not update the coordinate. It should, right?

Other feedbacks:

too much padding around the outside. Too much padding between Coordinates and Search button.
title Search Summary is redundant and a waste of real estate.

loitly · 2024-05-09T00:27:51Z

+                {title && <><Typography color={'warning'} level='title'>{title}</Typography></>}
+                <Typography color={'neutral'} level='body-md'>
+                    <Typography component='span' color={'primary'}>Search Type</Typography>: {searchType}
+                    {showRadius && searchType === 'Cone' && <>
+                        , <Typography component='span' color={'primary'}>Search Radius</Typography>: {radius}
+                    </>}


The combination of <></>, , and nested Typography is hard to look at. It would make it a lot easier to read if you define a simple function so you don't repeat yourself.

const keyVal = (k,v) => <>, <Typography component='span' color={'primary'}>{k}</Typography>: {v}</>;

I don't think component='span' is needed, but I guess you did it for a reason.

loitly · 2024-05-09T00:27:56Z

+        const searchType = doGetConeAreaOp() === CONE_CHOICE_KEY ? 'Cone' : (doGetConeAreaOp() === POLY_CHOICE_KEY  ? 'Polygon' : 'Multi-Object');
+        const coords = searchType === 'Cone' ? (reqObj?.UserTargetWorldPt || reqObj?.circleTarget) : (reqObj?.Polygon || reqObj?.['POS-polygon']);
+        const radius = reqObj?.radius || reqObj?.circleSize;
+
+        //in case of Multi-Object, get the fileName & rows
+        const fileName = searchType === 'Multi-Object' ? reqObj?.uploadInfo?.fileName : undefined;
+        const rows = searchType === 'Multi-Object' ? reqObj?.uploadInfo?.totalRows : undefined;
+
+        const defSlotProps = {
+            searchSummary: {
+                title: undefined,
+                searchType, coords, radius, fileName, rows,
+                showCoords: false,
+                showRadius: false,
+                showFileInfo: false,
+                sx:{}
+            },


I would not have put these logic here. These logic should be in SearchSummary instead.
Also, why would you need all of these props?
searchType, coords, radius, fileName, rows are calculated. When would you ever want to override it?
showCoords, showRadius, showFileInfo props are contingent on whether or not there's value in coords, radius, fileName. Why would you need flags?

About this, you're right about the first part. I need not add searchType, coords, radius, fileName, rows to searchSummary/defSlotProps as these are not meant to be overridden. So I'll just pass these directly to SearchSummary.

As for the second part, @robyww wanted the Search Summary to be customizable by whoever's calling it (for example: see how I'm calling it for Euclid here: https://github.com/IPAC-SW/irsa-ife/pull/323/files). DCE may want to call it differently, and for instance, may not want to show radius even when it's available. In fact, even though title does take up extra space, it can optionally be left empty as well.

That was the idea behind keeping the Search Summary customizable by any component calling EmbeddedPositionSearchPanel... But I am open to better ideas (or even a better default). Right now the default is to just show the Search Type (Cone/Polygon/Multi-object) and nothing else. And I can get rid of the title option altogether if it seems like there would be no good use case for it.

loitly · 2024-05-09T00:28:00Z

+                        direction={'row'}
+                        width={'100%'}
+                        slotProps={{
+                            searchBar: {px:0, py:0, justifyContent: 'left'},


isn't px:0, py:0 just p:0?

loitly · 2024-05-09T00:28:03Z

+                      else setIsHovered(true);
+             }}
+            style={{display: 'flex', flexDirection: 'column', alignItems: 'center', alignSelf: 'stretch', height:'100%',
+                         paddingBottom:insetSpacial?0:20, position: 'relative'}}>


This should be a Stack.

loitly · 2024-05-09T00:28:13Z

+                <CollapsibleGroup
+                    sx={{borderWidth: '0px',


CollapsibleGroup uses variant='outlined' by default. To remove border, you should set a different value, like variant='plain'.

loitly · 2024-05-09T00:28:26Z

+                        '& .MuiAccordionSummary-root': {
+                            paddingBlockStart: '0.5rem',
+                            paddingBlockEnd: '0.5rem',
+                            minBlockSize: '1rem',
+                            '&.Mui-expanded': {
+                                height: '1rem '
+                            }


I don't think these add any value except for additional padding which, in my opinion, is not needed. It however makes it really hard for someone else to modify months from now.

Okay so the reasoning behind this one is interesting. From what I can tell Joy/MUI's AccordionSummary assigns the following values:

paddingBlockStart: var(--ListItem-paddingY) paddingBlockEnd: var(--ListItem-paddingY) minBlockSize: var(--ListItem-minHeight)

which are inherited from the default JoyAccordionGroup as:

--ListItem-minHeight: 2.25rem; --ListItem-paddingY: 0.25rem; --ListItem-paddingX: 0.75rem;

Now I could potentially set these in ThemeSetup.js if we wanted to use these values (the ones I overrode) elsewhere too, but it actually looks good in other places, for example this one from Firefly Image tab (ImageSelect.jsx).

as the default.

Now the same (without me overriding the values) looks like this on Euclid (compare it with the test build):

which @robyww mentioned makes the header too big when the search panel is open.

So in order to make it smaller, I added this extra bit of styling. It's possible there may be simpler way to set the height, but I tried a few things and unless I overrode (or unchecked) this default style, my desired height for the header/summary was not respected.

Anyway, if we decide to keep this, I can add a small comment explaining this choice for future developer to understand at first glance.

kpuriIpac · 2024-05-09T20:16:26Z

Bug: Clicking around while in collapsed mode did not update the coordinate. It should, right?

Other feedbacks:

too much padding around the outside. Too much padding between Coordinates and Search button.

title Search Summary is redundant and a waste of real estate.

Oh yes that's a bug, thanks for pointing it out. the Header component does not rerender unless you do a mouse over (and then it updates the coordinates).

The padding between Coordinates and Search button is because coords is treated as one long string, and even after setting CSS like whiteSpace: 'normal', it still doesn't get rid of the extra space. Looking into both of these issues.

kpuriIpac · 2024-05-10T02:49:44Z

@loitly I addressed most of your feedback in the latest commit.

bug is fixed (by using useFieldGroupValue which allows component to re-render when the coords change by interacting with the Hips image)
- you can also the Cone Selection or Rectangular Selection tools when the panel is collapsed, and should observe the changes.
this allowed for better formatting of the text too. Especially for cone, I decided to use formatWorldPtToString which makes the summary much cleaner
for SearchSummary, I used a small variation of your suggested keyVal function, which allows me to account for the last entry (so as to not put a comma after that in the summary text).
finally, just need to fix up the style (padding) a bit more when the panel is collapsed and then I think then this will be ready, but let me know if you have any comments on these changes so far.
- also I think the Region Explorer panel is wider with changes made to IbeSearch in some other PR, so I'll try and fix this as well.

Test builds: Euclid & DCE (DCE is mostly to make sure the panel looks okay when expanded in DCE, it'll be empty when collapsed except for Search Type unless DCE wants to add more to search summary)

loitly

I confirm coordinates are updating while in collapsed mode now. Ok to merge.

loitly · 2024-05-10T21:14:40Z

+                    {keyValuePairs.map((pair, index) =>
+                        keyVal(pair.k, pair.v, index === keyValuePairs.length - 1)
+                    )}


I think you misunderstood. Now, it looks worse than before. What I meant is this,

{ keyVal('Search Type', searchType) } { showRadius && searchType === 'Cone' && keyVal('Search Radius', radius) } { showCoords && searchType !== 'Multi-Object' && coords && keyVal('Coordinates', coords) }

If you want to insert a , in between, add a <Stack divider={', '} direction='row'> around them. Of course, your keyVal function have to return an element and not fragment for this to work.
This is only to give you an idea of how it could have been done. As long as it's working for you, I am ok you leaving it as is.

Yeah this makes sense and the code is definitely cleaner using this suggested approach, but even after setting direction='row' for the divider stack, the formatting still looks kind of weird (I can add a screenshot if you'd like to take a look), so I'll leave this as is.

loitly · 2024-05-10T21:17:43Z

+                            <Slot component={SearchSummary} {...defSlotProps.searchSummary} searchType={searchType}
+                                  coords={coords} radius={radius} fileName={fileName}
+                                  rows={rows} slotProps={slotProps?.searchSummary}/>


This is exactly the same as when you left them in defSlotProps. Why move them out here?

loitly · 2024-05-10T21:19:58Z

+                  if (!isSearchPanel) setIsHovered(false);
+                  else setIsHovered(true);


Isn't this setIsHovered(isSearchPanel); ?

Yes you're right, I can make it simpler, it would be setIsHovered(isSearchPanel) (whatever the value of isSearchPanel is)

kpuriIpac · 2024-05-11T00:04:38Z

@robyww (For Monday May 13 - I'll ping you on Slack on Monday):

Made some more changes & cleanup after @loitly's feedback
- fixed a bug and it looks cleaner overall especially when the panel is collapsed
Was ready to merge, but noticed that search wasn't working for DCE when the panel is collapsed, and that's because I was not passing through the submitSearch method (that EmbeddedPositionSearchPanel requires to use use in its Header's FormPanel when collapsed). Fixed this in the latest commit, but please have a look at that.
- Also passing some slotProps from DCE for SearchSummary (showCoords:true, showRadius:true)
Euclid test build & DCE test build
I'll wait for final feedback/okay to merge because of this DCE change.

Note: Region Explorer's Search Panel looks a little too wide, this is due to a change in IbeSearchType where some of the input fields now have orientation='horizontal'..this works for Wise, but looks unclean when IbeSearchType is used like this inside a smaller dialog/panel. I'll fix this in a new Euclid ticket I've begun.

kpuriIpac · 2024-05-13T18:32:42Z

+    const Header = function(isOpen) {
+        const {groupKey} = useContext(FieldGroupCtx);
+        const reqObj = getFieldGroupResults(groupKey,true);


@loitly I think I get what you were trying to say earlier - instead of sending
searchType, coords, radius, fileName, rows to SearchSummary component, I could just send/use this request object directly in SearchSummary? is that right?

robyww · 2024-05-13T21:06:24Z

+        const [getTargetWp,]= useFieldGroupValue(targetKey);
+        const [getHiPSRadius,]= useFieldGroupValue(sizeKey);
+        const [getPolygon,]= useFieldGroupValue(polygonKey);
+
+        const userEnterWorldPt= () =>  parseWorldPt(getTargetWp());
+        const worldPt = formatWorldPtToString(userEnterWorldPt());
+
+        const searchType = doGetConeAreaOp() === CONE_CHOICE_KEY ? 'Cone' : (doGetConeAreaOp() === POLY_CHOICE_KEY  ? 'Polygon' : 'Multi-Object');
+        const coords = searchType === 'Cone' ? worldPt : getPolygon();
+        const radius = getHiPSRadius();
+
+        //in case of Multi-Object, get the fileName & rows
+        const fileName = searchType === 'Multi-Object' ? reqObj?.uploadInfo?.fileName : undefined;
+        const rows = searchType === 'Multi-Object' ? reqObj?.uploadInfo?.totalRows : undefined;
+
+        const defSlotProps = {
+            searchSummary: {
+                title: undefined,
+                showCoords: false,
+                showRadius: false,
+                showFileInfo: false,
+                searchType, coords, radius, fileName, rows,
+                sx:{}
+            },
+        };


All this work should be done in SearchSummary

for force a rerender use:

useFieldGroupRerender([targetKey,sizeKey,polygonKey,CONE_AREA_KEY]);

SearchSummary should only take a request

<Slot component={SearchSummary} request={reqObj}/>

SearchSummary is just a default implementation that will probably always be replaced. We may may need to make a helper components for replacing it but that will be in future steps.

SearchSummary's job is to take a request object and make the header UI.

robyww

Header and SearchSummary should be declared outside of the EmbeddedPositionSearchPanel. Move those out and then it will be fine to merge.

…ch summary and opacity

kpuriIpac added the enhancement label Apr 26, 2024

kpuriIpac self-assigned this Apr 26, 2024

kpuriIpac requested a review from robyww April 26, 2024 22:20

loitly reviewed Apr 29, 2024

View reviewed changes

kpuriIpac force-pushed the FIREFLY-1451-Embed-Panel-Enhanced branch 3 times, most recently from e227013 to ff63cad Compare May 2, 2024 02:36

robyww reviewed May 2, 2024

View reviewed changes

robyww added this to the 2024.2 milestone May 3, 2024

robyww added the UI Client side UI changes not related to any of the visualizers label May 3, 2024

kpuriIpac force-pushed the FIREFLY-1451-Embed-Panel-Enhanced branch from ff63cad to f61b9e2 Compare May 8, 2024 20:35

kpuriIpac commented May 8, 2024

View reviewed changes

kpuriIpac marked this pull request as ready for review May 8, 2024 20:42

loitly requested changes May 9, 2024

View reviewed changes

kpuriIpac force-pushed the FIREFLY-1451-Embed-Panel-Enhanced branch 4 times, most recently from 6520a59 to 2c426c1 Compare May 10, 2024 02:48

loitly approved these changes May 10, 2024

View reviewed changes

kpuriIpac commented May 13, 2024

View reviewed changes

robyww requested changes May 13, 2024

View reviewed changes

robyww approved these changes May 16, 2024

View reviewed changes

Firefly-1451: EmbeddedPositionSearchPanel enhancements including sear…

7765ab5

…ch summary and opacity

kpuriIpac force-pushed the FIREFLY-1451-Embed-Panel-Enhanced branch from ba74da4 to 7765ab5 Compare May 16, 2024 22:53

kpuriIpac merged commit af9ecf8 into dev May 16, 2024

kpuriIpac deleted the FIREFLY-1451-Embed-Panel-Enhanced branch May 16, 2024 22:53

		header: oneOfType([string, func, element]),
		expandedHeader: oneOfType([string, func, element]),

		if (!isSearchPanel) setIsHovered(false);
		else setIsHovered(true);

Conversation

kpuriIpac commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robyww left a comment

Choose a reason for hiding this comment

Uh oh!

kpuriIpac May 8, 2024 • 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!

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!

loitly May 9, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

kpuriIpac commented May 9, 2024

Uh oh!

kpuriIpac commented May 10, 2024 • 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!

loitly May 10, 2024 • 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 May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

All this work should be done in SearchSummary

SearchSummary should only take a request

Uh oh!

robyww left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kpuriIpac commented Apr 26, 2024 •

edited

Loading

kpuriIpac May 8, 2024 •

edited

Loading

loitly May 9, 2024 •

edited

Loading

kpuriIpac commented May 10, 2024 •

edited

Loading

loitly May 10, 2024 •

edited

Loading

kpuriIpac commented May 11, 2024 •

edited

Loading

All this work should be done in `SearchSummary`

`SearchSummary` should only take a request