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

Chore/release51 compatibility #61

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

olzzon
Copy link

@olzzon olzzon commented Jun 11, 2024

This PR is made on behalf of Superfly.

Current behavior:
The current version of Demo-blueprints did not work on Release51

Changes:
This PR is updated to support current development state of Release 51

Copy link
Member

@jstarpl jstarpl left a comment

Choose a reason for hiding this comment

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

Commendable effort! I'm just unsure about a couple of things that can be confusing to a first-time blueprint developer using this as a boilerplate.

actions: [
{
actions: {
[PlayoutActions.adlib]: {
Copy link
Member

Choose a reason for hiding this comment

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

I think this will technically work, but may be confusing to the reader? I think this should preferably be some sort of a stable ID.

Copy link
Contributor

@mint-dewit mint-dewit Jul 11, 2024

Choose a reason for hiding this comment

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

I agree with Jan's comment, it's a bit of an indirection. I would favor having an enum with the various action id's but even just a hardcoded string here would be more understandable to me.

{
},
triggers: {
[TriggerType.hotkey]: {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@@ -0,0 +1,7 @@
{
"extends": "./tsconfig.build.json",
Copy link
Member

Choose a reason for hiding this comment

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

Should this just reference ./packages/blueprints/tsconfig.build.json? The actual ./tsconfig.build.json seems to be incorrect anyway, since it has baseUrl as ./?

default: {
name: 'Default',
config: {
dvePresets: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a default 2box layout could be added here.

actions: [
{
actions: {
[PlayoutActions.adlib]: {
Copy link
Contributor

@mint-dewit mint-dewit Jul 11, 2024

Choose a reason for hiding this comment

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

I agree with Jan's comment, it's a bit of an indirection. I would favor having an enum with the various action id's but even just a hardcoded string here would be more understandable to me.

studioConfigManifest,
studioMigrations,
studioConfigSchema: JSONBlobStringify(ConfigSchema),
configPresets: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these config presets & playoutDevice configs could have better defaults to make it easier for new users to demo Sofie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants