From bb985eba3ae6cd2944522111c0cfd928b62b92bf Mon Sep 17 00:00:00 2001 From: Alex Kim <45559664+alex-kim-dev@users.noreply.github.com> Date: Wed, 2 Aug 2023 15:04:21 +0500 Subject: [PATCH] fix: resizing arrow labels (#6789) * fix arrow labels resizing - min arrow labels width based on font size - labels width and padding in % of container's width - resize labels simply multiplying by scale * remove no longer needed getContainerDims * fix arrow labels font size not updated on change font size action * fix bound arrows not updated right after resize * fix test * fix 3+ point arrow label resizing with shift * fix bound text not scaling when resizing with shift & n or s handle * fix arrow labels width not updating when moving a 2-point arrow point with shift --------- Co-authored-by: Aakansha Doshi --- src/components/App.tsx | 6 +- src/constants.ts | 3 + src/element/linearElementEditor.ts | 8 +-- src/element/resizeElements.ts | 99 +++++++++++++------------- src/element/textElement.ts | 45 ++++++------ src/element/textWysiwyg.tsx | 8 +-- src/tests/linearElementEditor.test.tsx | 8 +-- 7 files changed, 89 insertions(+), 88 deletions(-) diff --git a/src/components/App.tsx b/src/components/App.tsx index 4c202441..28d8401b 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -298,7 +298,6 @@ import { getApproxMinLineWidth, getBoundTextElement, getContainerCenter, - getContainerDims, getContainerElement, getDefaultLineHeight, getLineHeightInPx, @@ -3548,9 +3547,8 @@ class App extends React.Component { lineHeight, ); const minHeight = getApproxMinLineHeight(fontSize, lineHeight); - const containerDims = getContainerDims(container); - const newHeight = Math.max(containerDims.height, minHeight); - const newWidth = Math.max(containerDims.width, minWidth); + const newHeight = Math.max(container.height, minHeight); + const newWidth = Math.max(container.width, minWidth); mutateElement(container, { height: newHeight, width: newWidth }); sceneX = container.x + newWidth / 2; sceneY = container.y + newHeight / 2; diff --git a/src/constants.ts b/src/constants.ts index 4c6b67e2..6ef98af1 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -117,6 +117,7 @@ export const FRAME_STYLE = { export const WINDOWS_EMOJI_FALLBACK_FONT = "Segoe UI Emoji"; +export const MIN_FONT_SIZE = 1; export const DEFAULT_FONT_SIZE = 20; export const DEFAULT_FONT_FAMILY: FontFamilyValues = FONT_FAMILY.Virgil; export const DEFAULT_TEXT_ALIGN = "left"; @@ -239,6 +240,8 @@ export const VERSIONS = { } as const; export const BOUND_TEXT_PADDING = 5; +export const ARROW_LABEL_WIDTH_FRACTION = 0.7; +export const ARROW_LABEL_FONT_SIZE_TO_MIN_WIDTH_RATIO = 11; export const VERTICAL_ALIGN = { TOP: "top", diff --git a/src/element/linearElementEditor.ts b/src/element/linearElementEditor.ts index b10493e0..3426b3e1 100644 --- a/src/element/linearElementEditor.ts +++ b/src/element/linearElementEditor.ts @@ -264,11 +264,11 @@ export class LinearElementEditor { }; }), ); + } - const boundTextElement = getBoundTextElement(element); - if (boundTextElement) { - handleBindTextResize(element, false); - } + const boundTextElement = getBoundTextElement(element); + if (boundTextElement) { + handleBindTextResize(element, false); } // suggest bindings for first and last point if selected diff --git a/src/element/resizeElements.ts b/src/element/resizeElements.ts index b9fd6328..136c70fb 100644 --- a/src/element/resizeElements.ts +++ b/src/element/resizeElements.ts @@ -1,4 +1,4 @@ -import { SHIFT_LOCKING_ANGLE } from "../constants"; +import { MIN_FONT_SIZE, SHIFT_LOCKING_ANGLE } from "../constants"; import { rescalePoints } from "../points"; import { @@ -204,8 +204,6 @@ const rescalePointsInElement = ( } : {}; -const MIN_FONT_SIZE = 1; - const measureFontSizeFromWidth = ( element: NonDeleted, nextWidth: number, @@ -589,24 +587,42 @@ export const resizeSingleElement = ( }); } + if ( + isArrowElement(element) && + boundTextElement && + shouldMaintainAspectRatio + ) { + const fontSize = + (resizedElement.width / element.width) * boundTextElement.fontSize; + if (fontSize < MIN_FONT_SIZE) { + return; + } + boundTextFont.fontSize = fontSize; + } + if ( resizedElement.width !== 0 && resizedElement.height !== 0 && Number.isFinite(resizedElement.x) && Number.isFinite(resizedElement.y) ) { + mutateElement(element, resizedElement); + updateBoundElements(element, { newSize: { width: resizedElement.width, height: resizedElement.height }, }); - mutateElement(element, resizedElement); if (boundTextElement && boundTextFont != null) { mutateElement(boundTextElement, { fontSize: boundTextFont.fontSize, baseline: boundTextFont.baseline, }); } - handleBindTextResize(element, transformHandleDirection); + handleBindTextResize( + element, + transformHandleDirection, + shouldMaintainAspectRatio, + ); } }; @@ -722,12 +738,8 @@ export const resizeMultipleElements = ( fontSize?: ExcalidrawTextElement["fontSize"]; baseline?: ExcalidrawTextElement["baseline"]; scale?: ExcalidrawImageElement["scale"]; + boundTextFontSize?: ExcalidrawTextElement["fontSize"]; }; - boundText: { - element: ExcalidrawTextElementWithContainer; - fontSize: ExcalidrawTextElement["fontSize"]; - baseline: ExcalidrawTextElement["baseline"]; - } | null; }[] = []; for (const { orig, latest } of targetElements) { @@ -798,50 +810,39 @@ export const resizeMultipleElements = ( } } - let boundText: typeof elementsAndUpdates[0]["boundText"] = null; - - const boundTextElement = getBoundTextElement(latest); - - if (boundTextElement || isTextElement(orig)) { - const updatedElement = { - ...latest, - width, - height, - }; - const metrics = measureFontSizeFromWidth( - boundTextElement ?? (orig as ExcalidrawTextElement), - boundTextElement - ? getBoundTextMaxWidth(updatedElement) - : updatedElement.width, - boundTextElement - ? getBoundTextMaxHeight(updatedElement, boundTextElement) - : updatedElement.height, - ); - + if (isTextElement(orig)) { + const metrics = measureFontSizeFromWidth(orig, width, height); if (!metrics) { return; } - - if (isTextElement(orig)) { - update.fontSize = metrics.size; - update.baseline = metrics.baseline; - } - - if (boundTextElement) { - boundText = { - element: boundTextElement, - fontSize: metrics.size, - baseline: metrics.baseline, - }; - } + update.fontSize = metrics.size; + update.baseline = metrics.baseline; } - elementsAndUpdates.push({ element: latest, update, boundText }); + const boundTextElement = pointerDownState.originalElements.get( + getBoundTextElementId(orig) ?? "", + ) as ExcalidrawTextElementWithContainer | undefined; + + if (boundTextElement) { + const newFontSize = boundTextElement.fontSize * scale; + if (newFontSize < MIN_FONT_SIZE) { + return; + } + update.boundTextFontSize = newFontSize; + } + + elementsAndUpdates.push({ + element: latest, + update, + }); } const elementsToUpdate = elementsAndUpdates.map(({ element }) => element); - for (const { element, update, boundText } of elementsAndUpdates) { + for (const { + element, + update: { boundTextFontSize, ...update }, + } of elementsAndUpdates) { const { width, height, angle } = update; mutateElement(element, update, false); @@ -851,17 +852,17 @@ export const resizeMultipleElements = ( newSize: { width, height }, }); - if (boundText) { - const { element: boundTextElement, ...boundTextUpdates } = boundText; + const boundTextElement = getBoundTextElement(element); + if (boundTextElement && boundTextFontSize) { mutateElement( boundTextElement, { - ...boundTextUpdates, + fontSize: boundTextFontSize, angle: isLinearElement(element) ? undefined : angle, }, false, ); - handleBindTextResize(element, transformHandleType); + handleBindTextResize(element, transformHandleType, true); } } diff --git a/src/element/textElement.ts b/src/element/textElement.ts index 76de2c88..61813d2b 100644 --- a/src/element/textElement.ts +++ b/src/element/textElement.ts @@ -10,6 +10,8 @@ import { } from "./types"; import { mutateElement } from "./mutateElement"; import { + ARROW_LABEL_FONT_SIZE_TO_MIN_WIDTH_RATIO, + ARROW_LABEL_WIDTH_FRACTION, BOUND_TEXT_PADDING, DEFAULT_FONT_FAMILY, DEFAULT_FONT_SIZE, @@ -65,7 +67,7 @@ export const redrawTextBoundingBox = ( boundTextUpdates.text = textElement.text; if (container) { - maxWidth = getBoundTextMaxWidth(container); + maxWidth = getBoundTextMaxWidth(container, textElement); boundTextUpdates.text = wrapText( textElement.originalText, getFontString(textElement), @@ -83,13 +85,12 @@ export const redrawTextBoundingBox = ( boundTextUpdates.baseline = metrics.baseline; if (container) { - const containerDims = getContainerDims(container); const maxContainerHeight = getBoundTextMaxHeight( container, textElement as ExcalidrawTextElementWithContainer, ); - let nextHeight = containerDims.height; + let nextHeight = container.height; if (metrics.height > maxContainerHeight) { nextHeight = computeContainerDimensionForBoundText( metrics.height, @@ -155,6 +156,7 @@ export const bindTextToShapeAfterDuplication = ( export const handleBindTextResize = ( container: NonDeletedExcalidrawElement, transformHandleType: MaybeTransformHandleType, + shouldMaintainAspectRatio = false, ) => { const boundTextElementId = getBoundTextElementId(container); if (!boundTextElementId) { @@ -175,15 +177,17 @@ export const handleBindTextResize = ( let text = textElement.text; let nextHeight = textElement.height; let nextWidth = textElement.width; - const containerDims = getContainerDims(container); const maxWidth = getBoundTextMaxWidth(container); const maxHeight = getBoundTextMaxHeight( container, textElement as ExcalidrawTextElementWithContainer, ); - let containerHeight = containerDims.height; + let containerHeight = container.height; let nextBaseLine = textElement.baseline; - if (transformHandleType !== "n" && transformHandleType !== "s") { + if ( + shouldMaintainAspectRatio || + (transformHandleType !== "n" && transformHandleType !== "s") + ) { if (text) { text = wrapText( textElement.originalText, @@ -207,7 +211,7 @@ export const handleBindTextResize = ( container.type, ); - const diff = containerHeight - containerDims.height; + const diff = containerHeight - container.height; // fix the y coord when resizing from ne/nw/n const updatedY = !isArrowElement(container) && @@ -687,16 +691,6 @@ export const getContainerElement = ( return null; }; -export const getContainerDims = (element: ExcalidrawElement) => { - const MIN_WIDTH = 300; - if (isArrowElement(element)) { - const width = Math.max(element.width, MIN_WIDTH); - const height = element.height; - return { width, height }; - } - return { width: element.width, height: element.height }; -}; - export const getContainerCenter = ( container: ExcalidrawElement, appState: AppState, @@ -887,12 +881,19 @@ export const computeContainerDimensionForBoundText = ( return dimension + padding; }; -export const getBoundTextMaxWidth = (container: ExcalidrawElement) => { - const width = getContainerDims(container).width; +export const getBoundTextMaxWidth = ( + container: ExcalidrawElement, + boundTextElement: ExcalidrawTextElement | null = getBoundTextElement( + container, + ), +) => { + const { width } = container; if (isArrowElement(container)) { - return width - BOUND_TEXT_PADDING * 8 * 2; + const minWidth = + (boundTextElement?.fontSize ?? DEFAULT_FONT_SIZE) * + ARROW_LABEL_FONT_SIZE_TO_MIN_WIDTH_RATIO; + return Math.max(ARROW_LABEL_WIDTH_FRACTION * width, minWidth); } - if (container.type === "ellipse") { // The width of the largest rectangle inscribed inside an ellipse is // Math.round((ellipse.width / 2) * Math.sqrt(2)) which is derived from @@ -911,7 +912,7 @@ export const getBoundTextMaxHeight = ( container: ExcalidrawElement, boundTextElement: ExcalidrawTextElementWithContainer, ) => { - const height = getContainerDims(container).height; + const { height } = container; if (isArrowElement(container)) { const containerHeight = height - BOUND_TEXT_PADDING * 8 * 2; if (containerHeight <= 0) { diff --git a/src/element/textWysiwyg.tsx b/src/element/textWysiwyg.tsx index 9105ba70..a5bf7014 100644 --- a/src/element/textWysiwyg.tsx +++ b/src/element/textWysiwyg.tsx @@ -23,7 +23,6 @@ import { AppState } from "../types"; import { mutateElement } from "./mutateElement"; import { getBoundTextElementId, - getContainerDims, getContainerElement, getTextElementAngle, getTextWidth, @@ -177,20 +176,19 @@ export const textWysiwyg = ({ updatedTextElement, editable, ); - const containerDims = getContainerDims(container); let originalContainerData; if (propertiesUpdated) { originalContainerData = updateOriginalContainerCache( container.id, - containerDims.height, + container.height, ); } else { originalContainerData = originalContainerCache[container.id]; if (!originalContainerData) { originalContainerData = updateOriginalContainerCache( container.id, - containerDims.height, + container.height, ); } } @@ -214,7 +212,7 @@ export const textWysiwyg = ({ // autoshrink container height until original container height // is reached when text is removed !isArrowElement(container) && - containerDims.height > originalContainerData.height && + container.height > originalContainerData.height && textElementHeight < maxHeight ) { const targetContainerHeight = computeContainerDimensionForBoundText( diff --git a/src/tests/linearElementEditor.test.tsx b/src/tests/linearElementEditor.test.tsx index 6bcefb95..bcca0b1e 100644 --- a/src/tests/linearElementEditor.test.tsx +++ b/src/tests/linearElementEditor.test.tsx @@ -1128,7 +1128,7 @@ describe("Test Linear Elements", () => { height: 500, }); const arrow = UI.createElement("arrow", { - x: 210, + x: -10, y: 250, width: 400, height: 1, @@ -1152,8 +1152,8 @@ describe("Test Linear Elements", () => { expect( wrapText(textElement.originalText, font, getBoundTextMaxWidth(arrow)), ).toMatchInlineSnapshot(` - "Online whiteboard collaboration - made easy" + "Online whiteboard + collaboration made easy" `); const handleBindTextResizeSpy = vi.spyOn( textElementUtils, @@ -1165,7 +1165,7 @@ describe("Test Linear Elements", () => { mouse.moveTo(200, 0); mouse.upAt(200, 0); - expect(arrow.width).toBe(170); + expect(arrow.width).toBe(200); expect(rect.x).toBe(200); expect(rect.y).toBe(0); expect(handleBindTextResizeSpy).toHaveBeenCalledWith(