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(core): frame editor settings #9970

Merged
merged 8 commits into from
Feb 10, 2025

Conversation

OlegDev1
Copy link
Contributor

@OlegDev1 OlegDev1 commented Feb 5, 2025

Fixes #9646

Demo

Screen.Recording.2025-02-05.at.22.19.05.mov

Design Decisions

  • frame-tools.ts: other blocks update their props using one of the extensions in surface block. Since frame block is not a part of the surface, this extension wasn't called. I decided to update the props directly in the frame-tool.ts instead of writing an extension for entire frame block
  • snapshot.tsx: whenever editor settings are opened, one frame in each block preview area is deleted. This behavior deleted the preview frame that shows the selected color of frame settings. I decided to exclude this frame from deletion process

@OlegDev1 OlegDev1 requested a review from a team as a code owner February 5, 2025 21:40
Copy link

graphite-app bot commented Feb 5, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@graphite-app graphite-app bot requested review from a team February 5, 2025 21:40
@github-actions github-actions bot added mod:i18n Related to i18n app:core labels Feb 5, 2025
@graphite-app graphite-app bot requested a review from a team February 5, 2025 22:31
@github-actions github-actions bot added mod:infra Environment related issues and discussions docs Improvements or additions to documentation mod:env mod:component app:electron Related to electron app mod:dev app:server test Related to test cases rust mod:native mod:server-native labels Feb 5, 2025
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 53.84%. Comparing base (18513c6) to head (6513858).
Report is 8 commits behind head on canary.

Files with missing lines Patch % Lines
...ss/components/auto-complete/auto-complete-panel.ts 0.00% 2 Missing ⚠️
...te/blocks/src/root-block/edgeless/frame-manager.ts 0.00% 2 Missing ⚠️
...cks/src/root-block/edgeless/gfx-tool/frame-tool.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           canary    #9970      +/-   ##
==========================================
- Coverage   54.10%   53.84%   -0.27%     
==========================================
  Files        2279     2279              
  Lines      104475   104464      -11     
  Branches    17381    17332      -49     
==========================================
- Hits        56527    56245     -282     
- Misses      46571    46850     +279     
+ Partials     1377     1369       -8     
Flag Coverage Δ
server-test 78.18% <ø> (-0.70%) ⬇️
unittest 31.82% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@L-Sun L-Sun left a comment

Choose a reason for hiding this comment

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

Great job on the PR! I've noticed there are other entry points for adding affine:frame that might also need to be modified. Could you please take a look at those areas and make the necessary adjustments? Thank you for your hard work and contribution!

const id = this.crud.addBlock(
'affine:frame',
{
title: new Y.Text(`Frame ${frameIndex}`),
xywh: serializeXYWH(...xywh),
presentationIndex: frameMgr.generatePresentationIndex(),
},
surfaceBlockModel
);

private _addFrameBlock(bound: Bound) {
const surfaceModel = this.gfx.surface as SurfaceBlockModel;
const id = this.gfx.doc.addBlock(
'affine:frame',
{
title: new Text(new Y.Text(`Frame ${this.frames.length + 1}`)),
xywh: bound.serialize(),
index: this.gfx.layer.generateIndex(true),
presentationIndex: this.generatePresentationIndex(),
},
surfaceModel
);

@OlegDev1
Copy link
Contributor Author

OlegDev1 commented Feb 6, 2025

Great job on the PR! I've noticed there are other entry points for adding affine:frame that might also need to be modified. Could you please take a look at those areas and make the necessary adjustments? Thank you for your hard work and contribution!

const id = this.crud.addBlock(
'affine:frame',
{
title: new Y.Text(`Frame ${frameIndex}`),
xywh: serializeXYWH(...xywh),
presentationIndex: frameMgr.generatePresentationIndex(),
},
surfaceBlockModel
);

private _addFrameBlock(bound: Bound) {
const surfaceModel = this.gfx.surface as SurfaceBlockModel;
const id = this.gfx.doc.addBlock(
'affine:frame',
{
title: new Text(new Y.Text(`Frame ${this.frames.length + 1}`)),
xywh: bound.serialize(),
index: this.gfx.layer.generateIndex(true),
presentationIndex: this.generatePresentationIndex(),
},
surfaceModel
);

Thank you for the feedback, I really appreciate it! I have added nProps in frame-manager.ts, since this issue keeps appearing

graphite-app bot pushed a commit that referenced this pull request Feb 7, 2025
Related to #9970 (comment)

### What changes:
- Add missing zod shcema for edgeless basic props
- Change `applyLastProps` to generic function for better return type inference of
- Fix: add `ZodIntersection` case to `makeDeepOptional`
@OlegDev1
Copy link
Contributor Author

OlegDev1 commented Feb 7, 2025

@L-Sun I have this error after commit 459972f when starting in dev mode. But when I open it in a fresh browser (without any previous usage), it works fine. Is it ok?
Screenshot 2025-02-07 at 20 54 14

@OlegDev1 OlegDev1 force-pushed the feat-frame-settings branch from fa0a744 to d396ee6 Compare February 7, 2025 20:09
@OlegDev1
Copy link
Contributor Author

OlegDev1 commented Feb 7, 2025

I have updated the branch to the latest changes by rebasing it, since 459972f wasn't included. It seems I should've been updated it using another method, since my commit dates were updated. I know I can use merge, but will all merged commits will be included in this PR?

Could you please tell me the method you use to update the branch? Thanks

@OlegDev1
Copy link
Contributor Author

OlegDev1 commented Feb 7, 2025

There is a problem with editor settings preview. It started looking like this after update. I tried changing the frame from frame.json, but the preview remains the same, no matter how much I resize the frame

Screenshot 2025-02-07 at 21 49 04

@L-Sun
Copy link
Contributor

L-Sun commented Feb 7, 2025

There is a problem with editor settings preview. It started looking like this after update. I tried changing the frame from frame.json, but the preview remains the same, no matter how much I resize the frame

I'm sorry about that. It seems that the declarations for ZodSchema in the editor are currently only used for editor settings, not for model definitions. This can lead to some properties in the snapshot being overridden during initialization. I have reverted these changes in this PR.

@fundon fundon self-requested a review February 8, 2025 00:58
@OlegDev1
Copy link
Contributor Author

OlegDev1 commented Feb 8, 2025

There is a problem with editor settings preview. It started looking like this after update. I tried changing the frame from frame.json, but the preview remains the same, no matter how much I resize the frame

I'm sorry about that. It seems that the declarations for ZodSchema in the editor are currently only used for editor settings, not for model definitions. This can lead to some properties in the snapshot being overridden during initialization. I have reverted these changes in this PR.

Everything works great now. Thank you for your help!

@Brooooooklyn Brooooooklyn merged commit d4f0c53 into toeverything:canary Feb 10, 2025
60 of 63 checks passed
@OlegDev1 OlegDev1 deleted the feat-frame-settings branch February 10, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app:core app:electron Related to electron app app:server docs Improvements or additions to documentation mod:component mod:dev mod:env mod:i18n Related to i18n mod:infra Environment related issues and discussions mod:native mod:server-native rust test Related to test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

​[Improvement]: Support Frame backgroud color setting in Editor settings
5 participants