Skip to content

Firefly-1924: Add Legend support#1920

Open
robyww wants to merge 1 commit intodevfrom
FIREFLY-1924-legend
Open

Firefly-1924: Add Legend support#1920
robyww wants to merge 1 commit intodevfrom
FIREFLY-1924-legend

Conversation

@robyww
Copy link
Contributor

@robyww robyww commented Mar 5, 2026

Firefly-1924: Add Legend Support

  • added a legend on hips and images
  • new xs size for button
  • added selection in embedded components
  • added shortTitle for MOCs
  • some refactor and clean up
  • Renamed a couple of files to better reflect their function

ife PR: https://github.com/IPAC-SW/irsa-ife/pull/459

Testing

@robyww robyww added this to the 2026.1 milestone Mar 5, 2026
@robyww robyww self-assigned this Mar 5, 2026
@robyww robyww force-pushed the FIREFLY-1924-legend branch 3 times, most recently from 60f97ab to 752f899 Compare March 6, 2026 21:24
@robyww robyww marked this pull request as ready for review March 6, 2026 21:43
@robyww robyww requested review from kpuriIpac and lrebull March 6, 2026 21:43
@robyww robyww force-pushed the FIREFLY-1924-legend branch from 752f899 to 26ab615 Compare March 13, 2026 18:57
@robyww robyww requested a review from jaladh-singhal March 13, 2026 19:10
@lrebull
Copy link
Contributor

lrebull commented Mar 13, 2026

Nice!
DCE doesn't have the "details" button thingey in the legend - deliberate or accidental?
IRSA Viewer in general does have it, but the DCE tab within IV is also missing it.
The last iteration would tell you in the legend if the MOC was loading, and that was a really nice feature. It doesn't seem to be there anymore.

@robyww
Copy link
Contributor Author

robyww commented Mar 13, 2026

DCE doesn't have the "details" button thingey in the legend - deliberate or accidental?

I am not showing the details button when the MOC is used for a search interface, like dce, spherex, or euclid. For coverage views I enable it.

@robyww
Copy link
Contributor Author

robyww commented Mar 13, 2026

The last iteration would tell you in the legend if the MOC was loading, and that was a really nice feature. It doesn't seem to be there anymore.

I will look at this

@lrebull
Copy link
Contributor

lrebull commented Mar 13, 2026

are these not search interfaces?

Screenshot 2026-03-13 at 12 41 25 PM Screenshot 2026-03-13 at 12 41 20 PM

Comment on lines +210 to +213
if (viewProps.value && !viewProps.displayValue) {
// eslint-disable-next-line react-hooks/immutability
viewProps.displayValue = convertQuantityUnits(viewProps.value, viewProps.quantityBaseUnit, viewProps.unit);
}
Copy link
Member

Choose a reason for hiding this comment

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

I did something very similar but I'm firing the changes in store instead of modifying the stateful prop (viewProps.displayValue in here). Also check with 0 is important otherwise it gets ignored in falsy value check.

Following is what I tested and is working for my PR:

Suggested change
if (viewProps.value && !viewProps.displayValue) {
// eslint-disable-next-line react-hooks/immutability
viewProps.displayValue = convertQuantityUnits(viewProps.value, viewProps.quantityBaseUnit, viewProps.unit);
}
useEffect(() => {
// if value changed in the store (for e.g. by a setFieldValue()) and there's no displayValue, set it
if ((viewProps?.value || viewProps?.value===0) && viewProps?.unit && !viewProps?.displayValue) {
const newDisplayValue = convertQuantityUnits(viewProps.value, quantityBaseUnit, viewProps.unit);
fireValueChange({displayValue: newDisplayValue});
}
}, [viewProps?.value, viewProps?.unit, viewProps?.displayValue, fireValueChange, convertQuantityUnits, quantityBaseUnit]);

@robyww robyww force-pushed the FIREFLY-1924-legend branch 2 times, most recently from 881f701 to 4470715 Compare March 17, 2026 20:03
- include some reponse from feedback
@robyww robyww force-pushed the FIREFLY-1924-legend branch from 4470715 to fa56af2 Compare March 17, 2026 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants