-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
ricardoantoniocm
wants to merge
10
commits into
develop
Choose a base branch
from
fb-optic-2148/icons-in-the-circles-under-preannotation-regions-are-missing
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
761f855
fix: OPTIC-2148: Icons in the circles under preannotation regions are…
ricardoantoniocm 70f8e61
Adapts colors of interactive controls to brand.
ricardoantoniocm db72dc5
ensure icon svg is turned into a XML data URL
bmartel 964efeb
Merge branch 'develop' of https://github.com/heartexlabs/label-studio…
ricardoantoniocm c761d2b
Fixes icons not being visible on the Accept/Reject buttons. Improved …
ricardoantoniocm e8bbe9a
Biome fix.
ricardoantoniocm 18c5366
Adds Polyfill for TextEncoder and TextDecoder on jest.setup.js.
ricardoantoniocm 5c7ebba
Biome lint fix.
ricardoantoniocm 12d98ca
Applying Copilot suggestions. Setting iconImage width and height to 2…
ricardoantoniocm b3ec229
Merge branch 'develop' of https://github.com/heartexlabs/label-studio…
ricardoantoniocm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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,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; | ||
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(); | ||
|
||
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( | ||
/** | ||
|
@@ -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} | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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