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

[Feature] Add width & height to media detail model #361

Conversation

Dvergar
Copy link
Contributor

@Dvergar Dvergar commented Oct 5, 2023

This PR adds width & height to the model returned by the connector detail method. MediaDetail model is now diverging from Media (query method).

Related tickets

https://chilipublishintranet.atlassian.net/browse/EDT-1054

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Coverage report can be checked at https://chili-dev.azureedge.net/sdk/coverage/361/coverage.html

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Unit Test Results

    1 files    31 suites   24s ⏱️
284 tests 284 ✔️ 0 💤 0
287 runs  287 ✔️ 0 💤 0

Results for commit 66f2941.

♻️ This comment has been updated with latest results.

Copy link
Member

@pietervp pietervp left a comment

Choose a reason for hiding this comment

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

Also update https://github.com/chili-publish/studio-sdk/blob/main/types/MediaConnector.d.ts to reflect the same changes, would be good to review if the complete api surfacce still matches what we expect.

@Dvergar
Copy link
Contributor Author

Dvergar commented Oct 5, 2023

Also update https://github.com/chili-publish/studio-sdk/blob/main/types/MediaConnector.d.ts to reflect the same changes, would be good to review if the complete api surfacce still matches what we expect.

Ooops, you mentioned it in chat and i completely overlooked that.

So I updated both media & font connectors, we do not have preview (font) in getcapabilities so this probably needs to be added as well but i'll prefer not to do it in this ticket.

Probably some more cleanup related to that will be done in scope of >> https://chilipublishintranet.atlassian.net/browse/EDT-1172

@evan-mcgeek May I have your eyes 👀 on the font connector side?

PS: I'll split again MediaCapabilities/FontCapabilities in a follow-up ticket.

@Dvergar
Copy link
Contributor Author

Dvergar commented Oct 5, 2023

Just adding some findings.

We do have

export enum MediaDownloadType {
    LowResolutionWeb = 'lowresWeb',
    HighResolutionWeb = 'highresWeb',
}

Was it an intended constraint to omit outputVideo & outputPdf?
Is it a standard for you WRS to start an enum entry with an uppercase?

@Dvergar Dvergar requested a review from pietervp October 6, 2023 09:34
Copy link
Contributor

@evan-mcgeek evan-mcgeek left a comment

Choose a reason for hiding this comment

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

LGTM! The only thing left is to add a preview capability to the font connector

…to feature/EDT-1054-Update-connector-detail-models-with-width-and-height
Copy link
Member

@brapoprod brapoprod left a comment

Choose a reason for hiding this comment

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

seems non-breaking to me

@pietervp
Copy link
Member

pietervp commented Oct 9, 2023

Just adding some findings.

We do have

export enum MediaDownloadType {
    LowResolutionWeb = 'lowresWeb',
    HighResolutionWeb = 'highresWeb',
}

Was it an intended constraint to omit outputVideo & outputPdf? Is it a standard for you WRS to start an enum entry with an uppercase?

That was intentional, because WRS should only ever use one of those.

@Dvergar Dvergar merged commit 0644ee8 into main Oct 9, 2023
4 checks passed
@Dvergar Dvergar deleted the feature/EDT-1054-Update-connector-detail-models-with-width-and-height branch October 9, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants