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

Add embed code to video page #503

Merged
merged 4 commits into from
Aug 3, 2022

Conversation

JulianKniephoff
Copy link
Member

Closes #501.

Also includes some semi-related "fixes" to linting.

@github-actions
Copy link

github-actions bot commented Aug 3, 2022

🚀 This PR was deployed at https://pr503.tobira.opencast.org. The deployment will be updated whenever someone pushes onto this PR's branch.

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

I like this solution of using a button and modal. Works. The actual code seems to work fine too, without random stupid resolutions.

frontend/src/routes/Video.tsx Outdated Show resolved Hide resolved
Comment on lines 276 to 289
const embedCode = `<iframe ${[
`src="${CONFIG.opencast.presentationNode}/play/${id}"`,
"allowfullscreen",
`style="${[
"border: none;",
"width: 100%;",
"aspect-ratio: 16/9;",
].join(" ")}`,
'name="Player"',
'scrolling="no"',
'frameborder="0"',
'marginheight="0px"',
'marginwidth="0px"',
].join(" ")}></iframe>`;
Copy link
Member

Choose a reason for hiding this comment

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

What's with this wild mix of " and `? With join calls? Why not make all a big template string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because that string is shown, verbatim, in the UI. It's what people copy to where ever they need it to go. I don't want spaces and newlines in there.

Copy link
Member

Choose a reason for hiding this comment

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

Mh ok yes that makes sense. But I think we can do better in the code, no? I find it quite hard to read to be honest, the method calls and having all three kinds of stirng literals so close to one another is jarring.

One option I'd already prefer is:

`<iframe> `
+ `<allowfullscreen `
+ `<name="Player">`

Alternatively, I also wouldn't mind the big template string literal with .replace(/\n\s*/, ""). I think both improves readability. The missing " quote (see the other comment) is one indication that this is not too great to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

The missing " quote (see the other comment) is one indication that this is not too great to read.

Not quite. I saw my highlighting break, but thought it was an Emacs bug.

The other options all have other drawbacks, though. I literally went through all three of them. ;)

  • Replacing whitespace still makes the style attribute awkward if you want to line-break it (which I want).
  • The "concatenation method" in combination with our linting forces weird formatting and there is no way to configure the linting properly.

Copy link
Member

Choose a reason for hiding this comment

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

Two alternatives:

const embedCode = `<iframe `
    + `src="${CONFIG.opencast.presentationNode}/play/${id}" `
    + "allowfullscreen "
    + 'style="border: none; width: 100%; aspect-ratio: 16/9;" '
    + 'name="Player" '
    + 'scrolling="no" '
    + 'frameborder="0" '
    + 'marginheight="0px" '
    + 'marginwidth="0px" '
    + `></iframe>`;

const embedCode = `<iframe 
    src="${CONFIG.opencast.presentationNode}/play/${id}" 
    allowfullscreen 
    style="border: none; width: 100%; aspect-ratio: 16/9;" 
    name="Player" 
    scrolling="no" 
    frameborder="0" 
    marginheight="0px" 
    marginwidth="0px" 
></iframe>`.replace(/\n\s*/, " ");

I think both are a lot more readable than what we have here. But yes, they are both still not optimal. Rust's escaping newlines would help but oh well.

But I guess we won't find common ground here so I'll just merge it.

frontend/src/i18n/locales/de.yaml Outdated Show resolved Hide resolved
frontend/src/routes/Video.tsx Outdated Show resolved Hide resolved
@LukasKalbertodt LukasKalbertodt merged commit a1c8cdc into elan-ev:master Aug 3, 2022
@JulianKniephoff JulianKniephoff deleted the embed-code branch August 4, 2022 11:09
@LukasKalbertodt LukasKalbertodt added the changelog:user User facing changes label Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show embed code on video page
2 participants