Skip to content
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.

Sign up for GitHub

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

feat(user feedback): Adds toolbar for cropping and annotating #15282

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
+166 −87
Open

feat(user feedback): Adds toolbar for cropping and annotating #15282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
20de538
tool wip
c298lee Jan 31, 2025
5ba5800
use multiple class names in BEM format
c298lee Feb 3, 2025
ff07b6c
still works when option is off
c298lee Feb 3, 2025
71d8049
rm extra options
c298lee Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions packages/feedback/src/screenshot/components/CropIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import type { VNode, h as hType } from 'preact';

interface FactoryParams {
h: typeof hType;
}

export default function CropIconFactory({
h, // eslint-disable-line @typescript-eslint/no-unused-vars
}: FactoryParams) {
return function CropIcon(): VNode {
return (
<svg width="20" height="20" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg">
<path
d="M15.25 12.5H12.5M12.5 12.5H4.50001C3.94773 12.5 3.50001 12.0523 3.50001 11.5V3.50002M12.5 12.5L12.5 4.50002C12.5 3.94773 12.0523 3.50002 11.5 3.50002H3.50001M12.5 12.5L12.5 15.25M3.50001 3.50002V0.750031M3.50001 3.50002H0.75"
stroke="currentColor"
strokeWidth="1.5"
strokeLinecap="round"
strokeLinejoin="round"
/>
</svg>
);
};
}
2 changes: 1 addition & 1 deletion packages/feedback/src/screenshot/components/PenIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export default function PenIconFactory({
}: FactoryParams) {
return function PenIcon(): VNode {
return (
<svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg">
<svg width="20" height="20" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg">
<path
d="M8.5 12L12 8.5L14 11L11 14L8.5 12Z"
stroke="currentColor"
Expand Down
180 changes: 100 additions & 80 deletions packages/feedback/src/screenshot/components/ScreenshotEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-
import type * as Hooks from 'preact/hooks';
import { DOCUMENT, WINDOW } from '../../constants';
import CropCornerFactory from './CropCorner';
import CropIconFactory from './CropIcon';
import PenIconFactory from './PenIcon';
import { createScreenshotInputStyles } from './ScreenshotInput.css';
import { useTakeScreenshotFactory } from './useTakeScreenshot';
Expand Down Expand Up @@ -75,6 +76,7 @@ export function ScreenshotEditorFactory({
const useTakeScreenshot = useTakeScreenshotFactory({ hooks });
const CropCorner = CropCornerFactory({ h });
const PenIcon = PenIconFactory({ h });
const CropIcon = CropIconFactory({ h });

return function ScreenshotEditor({ onError }: Props): VNode {
const styles = hooks.useMemo(() => ({ __html: createScreenshotInputStyles(options.styleNonce).innerText }), []);
Expand All @@ -86,6 +88,7 @@ export function ScreenshotEditorFactory({
const [croppingRect, setCroppingRect] = hooks.useState<Box>({ startX: 0, startY: 0, endX: 0, endY: 0 });
const [confirmCrop, setConfirmCrop] = hooks.useState(false);
const [isResizing, setIsResizing] = hooks.useState(false);
const [isCropping, setIsCropping] = hooks.useState(true);
const [isAnnotating, setIsAnnotating] = hooks.useState(false);
Comment on lines +91 to 92
Copy link
Member

Choose a reason for hiding this comment

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

We might want to make these a single state where the value is more like an enum.
Right now i'm noticing spots like this:

setIsCropping(!isCropping);
setIsAnnotating(false);

which will only get more annoying if more tools get added. Not something for now, but when the next one gets added this'll be a fix.


hooks.useEffect(() => {
Expand Down Expand Up @@ -142,6 +145,10 @@ export function ScreenshotEditorFactory({
const croppingBox = constructRect(croppingRect);
ctx.clearRect(0, 0, imageDimensions.width, imageDimensions.height);

if (!isCropping) {
return;
}
Comment on lines +148 to +150
Copy link
Member

Choose a reason for hiding this comment

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

is this important to be after the clearRect() call? which rect is being cleared in this case

Copy link
Member Author

@c298lee c298lee Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

it is important to be after the clearRect. It removes the white border that's used to show that it's in the cropping state. Here's waht it would look like if it was before clearRect:
image


// draw gray overlay around the selection
ctx.fillStyle = 'rgba(0, 0, 0, 0.5)';
ctx.fillRect(0, 0, imageDimensions.width, imageDimensions.height);
Expand All @@ -154,7 +161,7 @@ export function ScreenshotEditorFactory({
ctx.strokeStyle = '#000000';
ctx.lineWidth = 1;
ctx.strokeRect(croppingBox.x + 3, croppingBox.y + 3, croppingBox.width - 6, croppingBox.height - 6);
}, [croppingRect]);
}, [croppingRect, isCropping]);

function onGrabButton(e: Event, corner: string): void {
setIsAnnotating(false);
Expand Down Expand Up @@ -398,102 +405,115 @@ export function ScreenshotEditorFactory({
return (
<div class="editor">
<style nonce={options.styleNonce} dangerouslySetInnerHTML={styles} />
{options._experiments.annotations && (
<div class="editor__tool-container">
<button
class="editor__pen-tool"
style={{
background: isAnnotating
? 'var(--button-primary-background, var(--accent-background))'
: 'var(--button-background, var(--background))',
color: isAnnotating
? 'var(--button-primary-foreground, var(--accent-foreground))'
: 'var(--button-foreground, var(--foreground))',
}}
onClick={e => {
e.preventDefault();
setIsAnnotating(!isAnnotating);
}}
<div class="editor__image-container">
<div class="editor__canvas-container" ref={canvasContainerRef}>
<div
class={`editor__crop-container ${isAnnotating ? 'editor__crop-container--inactive' : ''}
${confirmCrop ? 'editor__crop-container--move' : ''}`}
ref={cropContainerRef}
>
<PenIcon />
</button>
</div>
)}
<div class="editor__canvas-container" ref={canvasContainerRef}>
<div class="editor__crop-container" style={{ zIndex: isAnnotating ? 1 : 2 }} ref={cropContainerRef}>
<canvas onMouseDown={onDragStart} ref={croppingRef}></canvas>
{isCropping && (
<div>
<CropCorner
left={croppingRect.startX - CROP_BUTTON_BORDER}
top={croppingRect.startY - CROP_BUTTON_BORDER}
onGrabButton={onGrabButton}
corner="top-left"
></CropCorner>
<CropCorner
left={croppingRect.endX - CROP_BUTTON_SIZE + CROP_BUTTON_BORDER}
top={croppingRect.startY - CROP_BUTTON_BORDER}
onGrabButton={onGrabButton}
corner="top-right"
></CropCorner>
<CropCorner
left={croppingRect.startX - CROP_BUTTON_BORDER}
top={croppingRect.endY - CROP_BUTTON_SIZE + CROP_BUTTON_BORDER}
onGrabButton={onGrabButton}
corner="bottom-left"
></CropCorner>
<CropCorner
left={croppingRect.endX - CROP_BUTTON_SIZE + CROP_BUTTON_BORDER}
top={croppingRect.endY - CROP_BUTTON_SIZE + CROP_BUTTON_BORDER}
onGrabButton={onGrabButton}
corner="bottom-right"
></CropCorner>
</div>
)}
{isCropping && (
<div
style={{
left: Math.max(0, croppingRect.endX - 191),
top: Math.max(0, croppingRect.endY + 8),
}}
class={`editor__crop-btn-group ${confirmCrop ? 'editor__crop-btn-group--active' : ''}`}
>
<button
onClick={e => {
e.preventDefault();
if (croppingRef.current) {
setCroppingRect({
startX: 0,
startY: 0,
endX: croppingRef.current.width / DPI,
endY: croppingRef.current.height / DPI,
});
}
setConfirmCrop(false);
}}
class="btn btn--default"
>
{options.cancelButtonLabel}
</button>
<button
onClick={e => {
e.preventDefault();
applyCrop();
setConfirmCrop(false);
}}
class="btn btn--primary"
>
{options.confirmButtonLabel}
</button>
</div>
)}
</div>
<canvas
onMouseDown={onDragStart}
style={{ cursor: confirmCrop ? 'move' : 'auto' }}
ref={croppingRef}
class={`editor__annotation ${isAnnotating ? 'editor__annotation--active' : ''}`}
onMouseDown={onAnnotateStart}
ref={annotatingRef}
></canvas>
<CropCorner
left={croppingRect.startX - CROP_BUTTON_BORDER}
top={croppingRect.startY - CROP_BUTTON_BORDER}
onGrabButton={onGrabButton}
corner="top-left"
></CropCorner>
<CropCorner
left={croppingRect.endX - CROP_BUTTON_SIZE + CROP_BUTTON_BORDER}
top={croppingRect.startY - CROP_BUTTON_BORDER}
onGrabButton={onGrabButton}
corner="top-right"
></CropCorner>
<CropCorner
left={croppingRect.startX - CROP_BUTTON_BORDER}
top={croppingRect.endY - CROP_BUTTON_SIZE + CROP_BUTTON_BORDER}
onGrabButton={onGrabButton}
corner="bottom-left"
></CropCorner>
<CropCorner
left={croppingRect.endX - CROP_BUTTON_SIZE + CROP_BUTTON_BORDER}
top={croppingRect.endY - CROP_BUTTON_SIZE + CROP_BUTTON_BORDER}
onGrabButton={onGrabButton}
corner="bottom-right"
></CropCorner>
<div
style={{
left: Math.max(0, croppingRect.endX - 191),
top: Math.max(0, croppingRect.endY + 8),
display: confirmCrop ? 'flex' : 'none',
}}
class="editor__crop-btn-group"
>
</div>
</div>
{options._experiments.annotations && (
<div class="editor__tool-container">
<div />
Copy link
Member Author

@c298lee c298lee Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Extra div for reset button in a future PR

<div class="editor__tool-bar">
<button
class={`editor__tool ${isCropping ? 'editor__tool--active' : ''}`}
onClick={e => {
e.preventDefault();
if (croppingRef.current) {
setCroppingRect({
startX: 0,
startY: 0,
endX: croppingRef.current.width / DPI,
endY: croppingRef.current.height / DPI,
});
}
setConfirmCrop(false);
setIsCropping(!isCropping);
setIsAnnotating(false);
}}
class="btn btn--default"
>
{options.cancelButtonLabel}
<CropIcon />
</button>
<button
class={`editor__tool ${isAnnotating ? 'editor__tool--active' : ''}`}
onClick={e => {
e.preventDefault();
applyCrop();
setConfirmCrop(false);
setIsAnnotating(!isAnnotating);
setIsCropping(false);
}}
class="btn btn--primary"
>
{options.confirmButtonLabel}
<PenIcon />
</button>
</div>
<div />
Copy link
Member Author

@c298lee c298lee Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Extra div for colour picking in a future PR

</div>
<canvas
class="editor__annotation"
onMouseDown={onAnnotateStart}
style={{ zIndex: isAnnotating ? '2' : '1' }}
ref={annotatingRef}
></canvas>
</div>
)}
</div>
);
};
Expand Down
48 changes: 42 additions & 6 deletions packages/feedback/src/screenshot/components/ScreenshotInput.css.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,17 @@ export function createScreenshotInputStyles(styleNonce?: string): HTMLStyleEleme

style.textContent = `
.editor {
display: flex;
flex-grow: 1;
flex-direction: column;
}
.editor__image-container {
padding: 10px;
padding-top: 65px;
padding-bottom: 65px;
flex-grow: 1;
position: relative;
height: 100%;
border-radius: var(--menu-border-radius, 6px);

background-color: ${surface200};
background-image: repeating-linear-gradient(
Expand All @@ -34,6 +40,13 @@ export function createScreenshotInputStyles(styleNonce?: string): HTMLStyleEleme
);
}

.editor__annotation {
z-index: 1;
}
.editor__annotation--active {
z-index: 2;
}

.editor__canvas-container {
width: 100%;
height: 100%;
Expand All @@ -49,7 +62,15 @@ export function createScreenshotInputStyles(styleNonce?: string): HTMLStyleEleme
}

.editor__crop-container {
custor: auto;
position: absolute;
z-index: 2;
}
.editor__crop-container--inactive {
z-index: 1;
}
.editor__crop-container--move {
cursor: move;
}

.editor__crop-btn-group {
Expand All @@ -59,6 +80,10 @@ export function createScreenshotInputStyles(styleNonce?: string): HTMLStyleEleme
background: var(--button-background, var(--background));
width: 175px;
position: absolute;
display: none;
}
.editor__crop-btn-group--active {
display: flex;
}

.editor__crop-corner {
Expand Down Expand Up @@ -90,17 +115,28 @@ export function createScreenshotInputStyles(styleNonce?: string): HTMLStyleEleme
border-top: none;
}
.editor__tool-container {
position: absolute;
padding: 10px 0px;
top: 0;
padding-top: 8px;
display: flex;
justify-content: space-between;
}
.editor__pen-tool {
height: 30px;
.editor__tool-bar {
display: flex;
gap: 8px;
}
.editor__tool {
display: flex;
padding: 8px 12px;
justify-content: center;
align-items: center;
border: var(--button-border, var(--border));
border-radius: var(--button-border-radius, 6px);
background: var(--button-background, var(--background));
color: var(--button-foreground, var(--foreground));
}

.editor__tool--active {
background: var(--button-primary-background, var(--accent-background));
color: var(--button-primary-foreground, var(--accent-foreground));
}
`;

Expand Down
Loading