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

Jump to bottom

fix: restore text dimensions #5432

Merged
merged 5 commits into from Oct 28, 2022
Merged

fix: restore text dimensions #5432

merged 5 commits into from Oct 28, 2022

Conversation

dwelle
Copy link
Member

@dwelle dwelle commented Jul 10, 2022 •
edited

This PR makes sure that text dimensions are recalculated on restore, except during collaboration for perf reasons (we should take another look into rewriting our current text dimensions calculation to use the measureText).

  • changelog/readme

@dwelle dwelle requested a review from ad1992 July 10, 2022 16:13
@vercel
Copy link

vercel bot commented Jul 10, 2022 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
excalidraw ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Oct 22, 2022 at 6:19PM (UTC)
excalidraw-package-example ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Oct 22, 2022 at 6:19PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
docs ⬜️ Ignored (Inspect) Oct 22, 2022 at 6:19PM (UTC)

@DanielJGeiger
Copy link
Collaborator

This PR makes sure that text dimensions are recalculated on restore, except during collaboration for perf reasons (we should take another look into rewriting our current text dimensions calculation to use the measureText).

* [x]  changelog/readme

Reading through the code changes, this looks good overall. Perhaps rewriting text dimensions calculations would come after @excalidraw/plugins lands? Text-based plugins like MathJax or UML should be able to calculate dimensions themselves.

ad1992
ad1992 reviewed Jul 21, 2022
View reviewed changes
src/data/restore.ts
@@ -124,6 +125,7 @@ const restoreElementWithProperties = <

const restoreElement = (
element: Exclude<ExcalidrawElement, ExcalidrawSelectionElement>,
refreshDimensions = true,
Copy link
Member

@ad1992 ad1992 Jul 21, 2022 •
edited

Choose a reason for hiding this comment

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

Better to keep this false to avoid breaking changes ?

Copy link
Member Author

@dwelle dwelle Oct 22, 2022 •
edited

Choose a reason for hiding this comment

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

it's not a breaking change, it's a new default

nvm. But, whether the default is false or true doesn't matter, it'd still be a breaking change if we remove this in the future

@dwelle
Merge branch 'master' into restore_dimensions
85da80e 
# Conflicts:
#	src/element/newElement.ts
#	src/packages/excalidraw/CHANGELOG.md
#	src/tests/data/__snapshots__/restore.test.ts.snap
@dwelle
reduce API surface area by always refreshing dimensions for full `res…
28f9929 
…tore()`
@dwelle
Copy link
Member Author

dwelle commented Oct 22, 2022

I've removed the refreshDimensions param from restore() to reduce the API added, and reduce breaking changes if (once) we remove it in the future. For full state restore, perf is less likely to matter.

@dwelle dwelle merged commit c8f6e3f into master Oct 28, 2022
@dwelle dwelle deleted the restore_dimensions branch October 28, 2022 21:31
@dwelle dwelle mentioned this pull request Nov 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielJGeiger DanielJGeiger DanielJGeiger left review comments

@ad1992 ad1992 ad1992 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dwelle @DanielJGeiger @ad1992