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

feat(edgeless): support more fonts in canvas text #5339

Merged
merged 26 commits into from
Nov 20, 2023

Conversation

Flrande
Copy link
Member

@Flrande Flrande commented Nov 15, 2023

Close #5313

Breaking change in surface-model: bold -> fontWeight, italic -> fontStyle

By default, font files are loaded through CDN. If you want to reference local font files, you can modify this behaviour by overriding page-service (similar to #5158).

Copy link

vercel bot commented Nov 15, 2023

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

Name Status Preview Comments Updated (UTC)
blocksuite ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2023 4:28am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
blocksuite-docs ⬜️ Ignored (Inspect) Visit Preview Nov 20, 2023 4:28am

@zanwei
Copy link
Member

zanwei commented Nov 17, 2023

Looks good! @Flrande. However, there're some issues of UI

  1. The text content needs to be all left-aligned, and the font needs to use the font's own style to ensure that users have an expectation of what the font will look like before choosing it.
image
  1. If the font has only one font-weight, please disable the entry for selecting font-weight.
image image
  1. font-size issue still needs to fix
image

@Flrande Flrande marked this pull request as ready for review November 19, 2023 06:17
@doodlewind doodlewind added the breaking Contains breaking change that must be addressed label Nov 19, 2023
Copy link
Member

@doodlewind doodlewind left a comment

Choose a reason for hiding this comment

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

What will happen when opening existing canvas text element? I'd consider losing stying info is acceptable as long as no content lost or migration required.

@Flrande
Copy link
Member Author

Flrande commented Nov 19, 2023

What will happen when opening existing canvas text element? I'd consider losing stying info is acceptable as long as no content lost or migration required.

If migration can run normally, the original style will be preserved.

https://github.com/toeverything/blocksuite/pull/5339/files#diff-a66b1dd0ce968e0f79ba989797b3636ba818af20f3a69664115006a9d33d9aa3R119-R138

Do we need to add a fallback logic for unexecuted migration? (to maintain normal rendering at the cost of losing the style)

@doodlewind
Copy link
Member

I don't think we should do migration for this kind of UX upgrade. Given the status in AFFiNE, simply wiping away existing style sounds better as a tradeoff.

@Flrande
Copy link
Member Author

Flrande commented Nov 19, 2023

I don't think we should do migration for this kind of UX upgrade. Given the status in AFFiNE, simply wiping away existing style sounds better as a tradeoff.

If we simply remove the original styles, it means that fontWeight and fontStyle would need to become optional fields. However, this would lead to a wider range of code needing to adapt to it, which is difficult to say is a best practice. Canvas text is just one of the drawing elements under the surface, and even if the migration fails, its impact will only be limited to itself. The worst consequence is just that users may need to click the fontWeight and fontStyle settings buttons again, which isn't so severe that it becomes unusable. However, if we simply ignore the users' previous styles because of this update, it might lead to a worse user experience. For example, if a user has dozens of styled canvas texts, they would need to adjust them all back.

In short, I understand your concerns about the breaking changes to the data structure, but I believe it is different from the breaking changes of the AFFiNE GUID. Its impact is limited, and it will not lead to widespread rendering failures. Moreover, we will undergo multiple rounds of testing before releasing it. I think we don’t need to be so conservative at this point.

@doodlewind
Copy link
Member

I believe in this case, having the application layer compatible with legacy docs missing these new fields should be the expected tradeoff.

Given the cost scheduling and validating the migration, I am sure that text style does NOT deserve a breaking change. Migration should NOT happen more often than half a year.

And after all, due to the fragmented nature of local-first app, I even have the feeling that we will have to stop introducing ANY breaking changes - there is a famous slogan called "don't break the web", and the whole web APIs doesn't introduce breaking changes over DECADES.

@Flrande
Copy link
Member Author

Flrande commented Nov 19, 2023

after all, due to the fragmented nature of local-first app, I even have the feeling that we will have to stop introducing ANY breaking changes - there is a famous slogan called "don't break the web", and the whole web APIs doesn't introduce breaking changes over DECADES.

It sounds like taking a cold breath in.🥶

@doodlewind doodlewind removed the breaking Contains breaking change that must be addressed label Nov 19, 2023
@doodlewind
Copy link
Member

@Flrande no worries, we'll figure out alternatives preventing breaking changes :)

Current breaking changes will lead to a new CRDT workspace, which could be still be the escape hatch, but I'd consider this to be an exclusive feature for AFFiNE Cloud.

@doodlewind doodlewind merged commit 41a5711 into master Nov 20, 2023
18 checks passed
@doodlewind doodlewind deleted the flrande/feat/edgeless-font-1115 branch November 20, 2023 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable Major improvement worth emphasizing
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

Edgeless Toolbar Update
3 participants