Skip to content

fix: OPTIC-2148: Icons in the circles under pre-annotation regions are missing #7506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open
8 changes: 8 additions & 0 deletions web/libs/editor/jest.setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,11 @@ window.HTMLMediaElement.prototype.pause = function pauseMock() {
window.HTMLMediaElement.prototype.canPlayType = function canPlayTypeMock(type) {
return this._mock._supportsTypes.includes(type) ? "maybe" : "";
};

// Polyfill for TextEncoder and TextDecoder for Jest (Node 20+ has them, but jsdom may not expose them)
if (typeof global.TextEncoder === "undefined") {
global.TextEncoder = require("util").TextEncoder;
}
if (typeof global.TextDecoder === "undefined") {
global.TextDecoder = require("util").TextDecoder;
}
80 changes: 66 additions & 14 deletions web/libs/editor/src/components/ImageView/SuggestionControls.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { useCallback, useEffect, useState } from "react";
import { Circle, Group, Image, Layer, Rect } from "react-konva";
import { IconCheck, IconCross } from "@humansignal/icons";
import Konva from "konva";
import chroma from "chroma-js";
import { observer } from "mobx-react";
import { isDefined } from "../../utils/utilities";
import ReactDOMServer from "react-dom/server";
import React from "react";
import { IconCheck, IconCross } from "../../../../ui/src/assets/icons";

const getItemPosition = (item) => {
const { shapeRef: shape, bboxCoordsCanvas: bbox } = item;
Expand Down Expand Up @@ -73,15 +75,15 @@ export const SuggestionControls = observer(({ item, useLayer }) => {
<Rect x={0} y={0} width={64} height={32} fill="#000" cornerRadius={16} />
<ControlButton
onClick={() => item.annotation.rejectSuggestion(item.id)}
fill="#DD0000"
iconColor="#fff"
fill="#CC5E46"
iconColor="#FFFFFF"
icon={IconCross}
/>
<ControlButton
x={32}
onClick={() => item.annotation.acceptSuggestion(item.id)}
fill="#98C84E"
iconColor="#fff"
fill="#287A72"
iconColor="#FFFFFF"
Comment on lines +78 to +86
Copy link
Contributor

Choose a reason for hiding this comment

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

these colors will be the same for light and darkmode, is that intended? if we need separate colors we can look at getCurrentTheme() from web/libs/ui/src/lib/ThemeToggle/ThemeToggle.tsx

Copy link
Collaborator

Choose a reason for hiding this comment

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

and can we use css vars here?

Copy link
Contributor

Choose a reason for hiding this comment

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

dug into it a little, css var probably wont work because fill gets passed into a Konva Circle component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried CSS vars, it didn't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yyassi-heartex the colors look ok on both Light and Dark Mode even though we aren't using the correct vars.

That said, this will need more work because the icons themselves aren't visible and it seems they had not been visible for some time (issue seems unrelated to recent work). The above fix by Brandon unfortunately didn't address the issue.

@hlomzik is this something you could help us look into?

Copy link
Contributor

Choose a reason for hiding this comment

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

@hlomzik The problem here is simple, though the solution may not be.

The icons never were able to be rendered previously, because it was always requiring an image to be used for canvas, but our icons have always been SVG components through SVGR. So that means this never worked, because the resulting icons for as long as I have been here have been handled through SVGR, and would produce an SVG React component, not a base64 url or url to an svg.

Now, the solution here I would say we can't entertain as is, because it involves using React Server Rendering utilities which I am still surprised it even works on the client like this to be honest. As to what else could we do here, maybe have a special import of the svg which produces a url to the asset instead of a component with the svg inside, that might be a simpler approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the guidance @bmartel . I'll try that approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

these icons were definitely working at some point...
I see 2 options:

  • to modify imports as you mentioned to get the path to svg and add it via Konva.Image. doesn't SVGR allow to get the path or stringified svg?
  • to recreate this panel in a real svg, this might be more complicated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave this as unresolved for the time being. It's a low priority fix in any case.

In short, I was not successful in applying the recommended approach (removing React Server Rendering and attempting to pull icons as URLs) after many prompt iterations. Happy to jump back to it with some help.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, that's not a styling issue anymore, it requires some subtle but fundamential changes

icon={IconCheck}
/>
</Group>
Expand All @@ -100,21 +102,57 @@ export const SuggestionControls = observer(({ item, useLayer }) => {

const ControlButton = ({ x = 0, fill, iconColor, onClick, icon }) => {
const [img, setImg] = useState(new window.Image());
const imageSize = 16;
const imageSize = 20;
Copy link
Preview

Copilot AI May 13, 2025

Choose a reason for hiding this comment

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

[nitpick] The intrinsic icon dimensions are set to 12 (in useEffect) while the displayed image size is 20. Consider aligning these dimensions to avoid potential scaling artifacts.

Copilot uses AI. Check for mistakes.

const imageOffset = 32 / 2 - imageSize / 2;
const color = chroma(iconColor ?? "#fff");
const color = chroma(iconColor ?? "#FFFFFF");
const [hovered, setHovered] = useState(false);
const [animatedOpacity, setAnimatedOpacity] = useState(0.2);
const [animatedFill, setAnimatedFill] = useState("#fff");
const animationRef = React.useRef();

useEffect(() => {
const iconImage = new window.Image();

iconImage.onload = () => {
setImg(iconImage);
};
iconImage.width = 12;
iconImage.height = 12;
iconImage.src = icon;
}, [icon]);
iconImage.width = 20;
iconImage.height = 20;

const iconElement = React.createElement(icon, { color: iconColor, width: 20, height: 20 });
const svgString = ReactDOMServer.renderToStaticMarkup(iconElement);
const base64 = btoa(decodeURIComponent(encodeURIComponent(svgString)));
iconImage.src = `data:image/svg+xml;base64,${base64}`;
}, [icon, iconColor]);

useEffect(() => {
let start;
const duration = 150; // ms
const easeOut = (t) => 1 - (1 - t) ** 2;
const fromOpacity = animatedOpacity;
const toOpacity = hovered ? 1 : 0.2;
const fromFill = chroma(animatedFill);
const toFill = chroma(hovered ? fill : "#fff");

function animate(now) {
if (!start) start = now;
const elapsed = now - start;
const t = Math.min(1, elapsed / duration);
const eased = easeOut(t);
setAnimatedOpacity(fromOpacity + (toOpacity - fromOpacity) * eased);
setAnimatedFill(chroma.mix(fromFill, toFill, eased, "rgb").hex());
if (t < 1) {
animationRef.current = requestAnimationFrame(animate);
} else {
setAnimatedOpacity(toOpacity);
setAnimatedFill(toFill.hex());
}
}
cancelAnimationFrame(animationRef.current);
animationRef.current = requestAnimationFrame(animate);
return () => cancelAnimationFrame(animationRef.current);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [hovered, fill]);

const applyFilter = useCallback(
/**
Expand Down Expand Up @@ -142,10 +180,24 @@ const ControlButton = ({ x = 0, fill, iconColor, onClick, icon }) => {
width={32}
height={32}
onClick={onClick}
onMouseEnter={() => setHovered(true)}
onMouseLeave={() => setHovered(false)}
onMouseEnter={(e) => {
setHovered(true);
// Set cursor to pointer
const stage = e.target.getStage();
if (stage && stage.container()) {
stage.container().style.cursor = "pointer";
}
}}
onMouseLeave={(e) => {
setHovered(false);
// Reset cursor
const stage = e.target.getStage();
if (stage && stage.container()) {
stage.container().style.cursor = "";
}
}}
>
<Circle x={16} y={16} radius={14} opacity={hovered ? 1 : 0.2} fill={hovered ? fill : "#fff"} />
<Circle x={16} y={16} radius={14} opacity={animatedOpacity} fill={animatedFill} />
<Image
ref={(node) => applyFilter(node)}
x={imageOffset}
Expand Down
Loading