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 "share sketch" functionality to demo editor #205

Merged
merged 9 commits into from
Dec 9, 2021
Merged

Add "share sketch" functionality to demo editor #205

merged 9 commits into from
Dec 9, 2021

Conversation

jvfe
Copy link
Contributor

@jvfe jvfe commented Nov 10, 2021

  • Closes Feature ideas... URL encoding like Prof. Esperança does #143

  • Implements functionality specified in this comment

  • Inspired by similar solutions done in hydra and skulpt, this adds the following functionality:

    • A "Share" button to the demo editor, which creates a shareable URL to the current sketch, copies it to the clipboard and reruns the sketch.
    • This basically encodes the current editor code in base64 and implements a new URL parameter to contain the sketch (?sketch=).
    • Now when the page is loaded, target_sketch.js checks for the presence of this parameter and changes the sketch accordingly, decoding the sketch param into runnable pyp5js code.
    • The whole code for this functionality is included in a new Javascript file, share.js, which is loaded at the beginning of index.html.
  • I also took the liberty of separating HTML, CSS and JS in the demo editor, as I believe this will make future work much easier - separation of concerns does help a lot.

  • Test it here

  • Example of a shared sketch URL

  • Now for some problems with this current approach:

    • While it is, essentially, the same approach that's used in hydra, hydra code usually contains less characters than pyp5js.
    • Therefore, the current format leads to very large URLs, especially with larger sketches, which aren't very nice to look at. I don't really have any ideas at the moment on how to solve this problem so I'm all ears.

Now pyodide is loaded with the shared sketch instead of always
defaulting to the base sketch
Increases the separation of concerns and makes it easier to implement
new features and fixes.
Should avoid issues with GH's domain
Copy link
Owner

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

@jvfe the usability of the share button is amazing!!!! Thanks for putting effort on that. This will be extremely helpful for teachers to user the demo editor to share their experiments with students, right @villares? Anyway, I left a few comments on small changes to be done in this PR.

Here's another test and it's working nice.

@villares can you experiment on @jvfe demo editor and give us your feedback about this? I think you have already faced more usage scenarios than I can imagine =)

ellipse(100, 100, diameter, diameter)

`;
let userCode = checkForSketch();
Copy link
Owner

Choose a reason for hiding this comment

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

We shouldn't change the target_sketch.js to call this function because this should be exclusively of the demo editor. My idea would be for the JS from the demo editor to be call this checkForSketch function and update both sketch-holder and the IDE code with that.

I know that this can prevent the sketch from automatically load itself, but I don't see a problem to request the user to click on the Run button to see the sketch running.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I fell that we could update this line to just be let userCode = "". This hard coded user code is probably a legacy from the very first implementations of pyodide mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, I'll send this change, had thought of removing it as well but tried to keep things as they were for the most part.

@@ -0,0 +1,77 @@
const initialSketch = checkForSketch();
Copy link
Owner

Choose a reason for hiding this comment

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

Nice touch on moving this to a separated js file.

Copy link
Owner

Choose a reason for hiding this comment

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

Since we're using jQuery here, shouldn't we wrap this whole Javascript inside of a $(document).ready() callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going from what I can see in view_sketch.html the editor config should be outside of this call.
Either way, I'll send a commit changing this, as most of the code in afterBody should be included in this call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in ba89c38 but I used the vanilla JS "DOMContentLoaded" which should have the same effect.

<script>hljs.initHighlightingOnLoad();</script>

<link rel="stylesheet" href="styles.css">
</head>
Copy link
Owner

Choose a reason for hiding this comment

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

It would be nice to have the docs/pyodide/index.html.template updated as well because this index.html is generated using that template with the make refresh_demo command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think I did it in d1cf2e2 but I might've missed something.

Now there is no base sketch to be loaded together with the page.
Updates the demo editor template with the changes implemented when the
share sketch button was added
Updates the HTML and CSS for the pyodide demo to be similar to the local
server, such as buttons and code editor functions.

- Add collapse button
- Add styling from basscss
- Remove unused dependencies, such as jquery and highlightjs
@jvfe
Copy link
Contributor Author

jvfe commented Dec 9, 2021

Hi! Was taking a look at my open pull requests and something I wanted to do but totally forgot in this one was update the HTML/CSS of the page so it functions like the local server - better looking buttons, collapsible code editor, etc - so that's what
686fac9 is for, it doesn't change the share sketch functionality. If it would be better to include it in another PR just say so, seemed better to include it in this since the changes are pretty minor and I was already working on the demo editor.

Edit: I believe the failing CI is unrelated to my changes.

Copy link
Owner

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jvfe for this contribution. Now this PR is way more focused only in the demo editor and, from what I tested locally, it's working as expected. This is a HUGE feature to have =)

@berinhard berinhard merged commit 3b9902e into berinhard:develop Dec 9, 2021
@jvfe jvfe deleted the feat/issue_143 branch December 9, 2021 22:42
@villares
Copy link
Contributor

Thank you so much @jvfe !

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.

Feature ideas... URL encoding like Prof. Esperança does
3 participants