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.

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

fix: set height correctly when text properties updated while editing in container until first submit #4469

Merged
merged 2 commits into from
Dec 23, 2021

Conversation

ad1992
Copy link
Member

@ad1992 ad1992 commented Dec 23, 2021

Prev
Excalidraw (36)

Now

Excalidraw (37)

fixes #4461
This only happens for first time since text editor width is not yet set

@vercel
Copy link

vercel bot commented Dec 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/J6XXoBA1ZymCsiYaw9WAiPPw7QqJ
✅ Preview: https://excalidraw-git-aakansha-prop-excalidraw.vercel.app

Copy link
Collaborator

@zsviczian zsviczian left a comment

Choose a reason for hiding this comment

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

Why have the (almost) same code 4 times? Why not just change redrawTextBoundingBox like the below and leave the rest unchanged?

export const redrawTextBoundingBox = (element: ExcalidrawTextElement) => {
  let maxWidth;
  if (element.containerId) {
    const container = Scene.getScene(element)!.getElement(
      element.containerId,
    );
    maxWidth = container?container.width - PADDING * 2:undefined;
  }

....

@dwelle
Copy link
Member

dwelle commented Dec 23, 2021

Why have the (almost) same code 4 times? Why not just change redrawTextBoundingBox like the below and leave the rest unchanged?

A good point, but we can't do that because the cloned element passed into redrawTextBoundingBox() doesn't yet have a <ExcalidrawElement, Scene> mapping.

@ad1992
Copy link
Member Author

ad1992 commented Dec 23, 2021

Why have the (almost) same code 4 times? Why not just change redrawTextBoundingBox like the below and leave the rest unchanged?

A good point, but we can't do that because the cloned element passed into redrawTextBoundingBox() doesn't yet have a <ExcalidrawElement, Scene> mapping.

Yep, other approaches could be to

  1. Pass original element so scene could be derived inside redrawTextBoundingBox which probably is n't semantically correct API wise since purpose isn't clear.
  2. Calculate the width of the DOM element which is not good perf wise and not needed after first submit at all

@ad1992 ad1992 merged commit ef62390 into master Dec 23, 2021
@ad1992 ad1992 deleted the aakansha-prop branch December 23, 2021 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

initial font-size change results in too large height and unaligned text
3 participants