-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: develop
Are you sure you want to change the base?
Changes from 8 commits
761f855
70f8e61
db72dc5
964efeb
c761d2b
e8bbe9a
18c5366
5c7ebba
12d98ca
b3ec229
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||||||
|
@@ -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" | ||||||
icon={IconCheck} | ||||||
/> | ||||||
</Group> | ||||||
|
@@ -100,10 +102,13 @@ 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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Positive FeedbackNegative Feedback |
||||||
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(); | ||||||
|
@@ -113,8 +118,41 @@ const ControlButton = ({ x = 0, fill, iconColor, onClick, icon }) => { | |||||
}; | ||||||
iconImage.width = 12; | ||||||
iconImage.height = 12; | ||||||
iconImage.src = icon; | ||||||
}, [icon]); | ||||||
|
||||||
const iconElement = React.createElement(icon, { color: iconColor, width: 12, height: 12 }); | ||||||
const svgString = ReactDOMServer.renderToStaticMarkup(iconElement); | ||||||
const base64 = btoa(unescape(encodeURIComponent(svgString))); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of 'unescape' is discouraged as it is deprecated. Consider using an alternative method for handling Unicode characters when encoding the SVG to base64.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
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( | ||||||
/** | ||||||
|
@@ -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} | ||||||
|
There was a problem hiding this comment.
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()
fromweb/libs/ui/src/lib/ThemeToggle/ThemeToggle.tsx
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
componentThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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