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

Asciinema support #1653

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

Asciinema support #1653

wants to merge 10 commits into from

Conversation

jessp01
Copy link
Contributor

@jessp01 jessp01 commented Jul 25, 2023

asciinema is a free and open source solution for recording terminal sessions and sharing them on the web.

Since asciinema is a casting format that's basically a text file rather than a video (see https://github.com/asciinema/asciinema/blob/main/doc/asciicast-v2.md), functions like volume and mute are irrelevant.

Important note: In order to correctly render the player, a CSS is needed (see https://github.com/asciinema/asciinema-player/releases/download/v3.5.0/asciinema-player.css). I couldn't find a similar case in any of the other players so I wanted to consult with you as to where you feel we should commit it (or maybe you reckon it's better to include a link tag in the render() function?). For now, I committed it under src/demo/asciinema-player.css just to make the demo app work but of course, that should be changed.

In the demo app, you'll also notice that the player's div doesn't occupy the whole width of its parent even though the style is set thusly:

    render () { 
      const style = {
        width: '100%',
        height: '100%'
      }    
      return (
        <div style={style} ref={this.ref} id={this.playerID} />
      )    
    }

more info about the reason for that can be found here: https://github.com/asciinema/asciinema-player#fit

@jessp01 jessp01 mentioned this pull request Jul 26, 2023
Merged
@jessp01
Copy link
Contributor Author

jessp01 commented Aug 6, 2023

Hi @cookpete ,

Any thoughts on this? I really do think it could benefit react-player's users.

Cheers,

@jessp01 jessp01 mentioned this pull request Aug 30, 2023
Merged
@heff
Copy link
Collaborator

heff commented Sep 6, 2023

Hey 👋 , super cool video use case and tech. I'm still wrapping my head around the architecture of react-player, but I know one of the hesitancies Pete has had in merging more playback types is the weight of the default bundle. If you have any insights on that topic let me know. Otherwise I'll weigh in more once I understand the situation better.

@cookpete
Copy link
Owner

cookpete commented Sep 7, 2023

I think for the time being it’s fine to merge these extra players, with maybe some emphasis in the Usage part of the readme to make it clear that importing react-player will import every player and inflate your bundle, regardless of whether you use it or not. There is already a suggestion to use individual players (eg react-player/youtube) or react-player/lazy but maybe this needs to be more prominent somehow.

@jessp01
Copy link
Contributor Author

jessp01 commented Sep 7, 2023

Hi @heff , @cookpete ,

I see some references to that already. To me, it's clear enough but if you feel additional clarifications should be made then of course, I'll all for it.

With regards to ASCIInema specifically, the only question I have (as per my original comment) is:

In order to correctly render the player, a CSS is needed (see https://github.com/asciinema/asciinema-player/releases/download/v3.5.0/asciinema-player.css). I couldn't find a similar case in any of the other players so I wanted to consult with you as to where you feel we should commit it (or maybe you reckon it's better to include a link tag in the render() function?). For now, I committed it under src/demo/asciinema-player.css just to make the demo app work but of course, that should be changed.

Could you share your thoughts?

Cheers,

@cookpete
Copy link
Owner

Hey @jessp01

I see two options:

  • Add something to the readme (or somewhere appropriate) to explain that you will need to include asciinema-player.css somewhere in your bundle for the player to work – leave it up to the dev to decide how to load it.
  • Maybe add a loadStyles util, similar to getSDK, which adds a <link> tag to the <head> if it has not already been added. Then when you call getSDK inside the load() method, you can also do loadStyles(STYLESHEET_URL) or similar

@jessp01
Copy link
Contributor Author

jessp01 commented Sep 18, 2023

Hey @cookpete ,

Add something to the readme (or somewhere appropriate) to explain that you will need to include asciinema-player.css somewhere in your bundle for the player to work – leave it up to the dev to decide how to load it.

If I could trust everyone to read, I'd vote for that but, as I think you'll agree, being well experienced, people mostly don't and then, they report issues... so, I reckon adding a loadStyles() util method is best (and could potentially serve other purposes in future). Do you want to do so or shall I?

Cheers,

@jessp01
Copy link
Contributor Author

jessp01 commented Apr 22, 2024

Hi all,

Any news on this?

Cheers,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jessp01 @heff @cookpete