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

[EDT-1765] SnapshotSettings introduced #549

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

[EDT-1765] SnapshotSettings introduced #549

wants to merge 23 commits into from

Conversation

evan-mcgeek
Copy link
Contributor

@evan-mcgeek evan-mcgeek commented Dec 19, 2024 •
edited
Loading

This PR adds a new method getSnapshotWithSettings() with SnapshotSettings object to be able to control snapshot properties, eg. increase/decrease its resolution

PR Guidelines

Related tickets

@evan-mcgeek evan-mcgeek self-assigned this Dec 19, 2024
@evan-mcgeek evan-mcgeek requested review from a team as code owners December 19, 2024 13:58
@github-actions GitHub Actions
Copy link
Contributor

Coverage Report

Coverage report can be checked at https://stgrafxstudiodevpublic.blob.core.windows.net/sdk/coverage/549/coverage.html

Use PR sdk package

Tarball can be downloaded from https://stgrafxstudiodevpublic.blob.core.windows.net/sdk/dev-packages/549/studio-sdk.tgz

To use in local project, change package.json to use local tarball

"dependencies": {
    "@chili-publish/studio-sdk": "https://stgrafxstudiodevpublic.blob.core.windows.net/sdk/dev-packages/549/studio-sdk.tgz"
}

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 19, 2024 •
edited
Loading

Unit Test Results

    1 files    40 suites   45s ⏱️
449 tests 449 ✔️ 0 💤 0 ❌
454 runs  454 ✔️ 0 💤 0 ❌

Results for commit e5dbe94.

♻️ This comment has been updated with latest results.

@sebastianpujina sebastianpujina added the Ready for QA run automated tests label Jan 3, 2025
@alexoparii alexoparii added Ready for QA run automated tests and removed Ready for QA run automated tests labels Jan 7, 2025
Dvergar
Dvergar reviewed Jan 13, 2025
View reviewed changes
Copy link
Contributor

@Dvergar Dvergar left a comment

Choose a reason for hiding this comment

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

Why isn't getSnapshot kept with a nullable settings parameter instead?

@evan-mcgeek
Copy link
Contributor Author

Why isn't getSnapshot kept with a nullable settings parameter instead?

good idea indeed

evan-mcgeek and others added 10 commits January 16, 2025 11:28
@evan-mcgeek
[EDT-1765] SnapshotSettings introduced
b127c20
@evan-mcgeek
[EDT-1765] reverted old method
0ae9ff9
@NicolaVerbeeck @evan-mcgeek
[Fix][EDT-1745] Fix missing configuration (#554)
2fa7395
@evan-mcgeek
CI: bumps version to 1.18.1-rc.4 [skip ci]
5418ad3
@abdelhalimkhouas @evan-mcgeek
[FIX] Add smartCrop and manualCrop fitModes (#556)
6df0d5b 
add smartCrop and manualCrop fitModes
@evan-mcgeek
CI: bumps version to 1.18.1-rc.5 [skip ci]
79d086a
@evan-mcgeek
[Feature][EDT-1762] layout display name (#553)
1d51bfe 
* [EDT-1762] get / set / reset layout display name methods

* [EDT-1762] unit tests

* [EDT-1762] displayName added to DTO

* [EDT-1762] privateData added to DTO

* [EDT-1762] prettier

* [EDT-1762] non-nullable display name
@evan-mcgeek
CI: bumps version to 1.18.1-rc.6 [skip ci]
e85c4fc
@evan-mcgeek
[EDT-1765] nullable SnapshotSettings instead of deprecation
0a39818
@evan-mcgeek
Merge remote-tracking branch 'origin/feature/EDT-1765-major-axis-size…
c8a5f72 
…' into feature/EDT-1765-major-axis-size
Dvergar
Dvergar previously approved these changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@Dvergar Dvergar left a comment

Choose a reason for hiding this comment

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

nit, I would maybe describe what is the behavior of a null largestAxisSize and if there is an enforced limit from our side.

Dvergar
Dvergar previously approved these changes Feb 3, 2025
View reviewed changes
Matthiee
Matthiee previously approved these changes Feb 3, 2025
View reviewed changes
Dvergar
Dvergar previously approved these changes Feb 5, 2025
View reviewed changes
@evan-mcgeek evan-mcgeek dismissed stale reviews from Dvergar and Matthiee via 0bd9e50 February 6, 2025 13:54
@evan-mcgeek evan-mcgeek added Ready for QA run automated tests and removed Ready for QA run automated tests labels Feb 6, 2025
@evan-mcgeek evan-mcgeek enabled auto-merge (squash) February 6, 2025 14:08
Matthiee
Matthiee previously approved these changes Feb 6, 2025
View reviewed changes
@evan-mcgeek evan-mcgeek requested a review from Matthiee February 6, 2025 15:33
@evan-mcgeek evan-mcgeek added Ready for QA run automated tests and removed Ready for QA run automated tests labels Feb 6, 2025
Matthiee
Matthiee reviewed Feb 7, 2025
View reviewed changes
packages/sdk/src/controllers/PageController.ts
const res = await this.#blobAPI;
return res.getPageSnapshot(pageId).then((result) => (result as Uint8Array) ?? (result as EditorResponse<null>));
return res
.getPageSnapshotWithSettings(pageId, JSON.stringify(settings))
Copy link
Member

Choose a reason for hiding this comment

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

Only perform JSON.stringify if settings are defined

Copy link
Member

Choose a reason for hiding this comment

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

getSnapshotWithSettings('0', 'null') will throw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Matthiee Matthiee Matthiee approved these changes

@brapoprod brapoprod brapoprod left review comments

@alexandraFlavia9 alexandraFlavia9 Awaiting requested review from alexandraFlavia9 alexandraFlavia9 is a code owner automatically assigned from chili-publish/react

@abdelhalimkhouas abdelhalimkhouas Awaiting requested review from abdelhalimkhouas abdelhalimkhouas is a code owner automatically assigned from chili-publish/react

@smereuta smereuta Awaiting requested review from smereuta smereuta is a code owner automatically assigned from chili-publish/react

@Dvergar Dvergar Awaiting requested review from Dvergar

At least 2 approving reviews are required to merge this pull request.

Assignees

@evan-mcgeek evan-mcgeek

Labels
Ready for QA run automated tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@evan-mcgeek @Dvergar @Matthiee @brapoprod @alexoparii @sebastianpujina @NicolaVerbeeck @abdelhalimkhouas