Created a loading indicator when the canvas is being set up - #8997
Conversation
|
Hi @Jextic thanks for putting this together and sorry for the long time to get to your comment, but please check the comment on the issue. If possible, to keep (1) non text based and (2) decoupled. I will be able to review this more deeply next week as well. Thanks for your work on this! |
…g text with a basic animation.
…unit test for the loading indicator to work with the updated loading.js file.
|
Hi @ksen0, loading-animation-test.mp4 |
|
Nice, thanks! Looks good to me. |
Yes that one is known to be flaky, on CI it will pass when rerunning. |
| if (!document.getElementById('loading-style')) { | ||
| const loadingStyle = document.createElement('style'); | ||
| loadingStyle.id = 'loading-style'; | ||
| loadingStyle.textContent = '@keyframes loading-spin { to { transform: rotate(360deg); } }'; |
There was a problem hiding this comment.
Would like this animation name to be namespaced a bit more, something like p5-loading-spin.
There was a problem hiding this comment.
Thank you. I've changed the name from loading-spin to p5-loading-spin in my most recent commits.
| lifecycles.presetup = function() { | ||
| if (typeof window === 'undefined' || loadingIndicator) return; | ||
|
|
||
| const container = this.canvas?.parentElement || document.body; |
There was a problem hiding this comment.
I've not tested this out yet but how does this look on instance mode sketches with contents already on the page and the sketch attached via canvas.parent()?
Also what about multiple sketches in instance mode?
There was a problem hiding this comment.
Thanks for taking the time to review. I have tested it out and if there is already existing contents on the page, they won't be affected. A loading indicator will appear during presetup and will only remove itself during postsetup. I also updated presetup so that the loading indicator is now independent for each sketch so it is compatible if there are multiple sketches in instance mode.
I realize that this.canvas?.parentElement might never be called on since the canvas is created during setup() so I removed it. If the sketch is created using new p5(sketch);, a loading indicator will appear at the center of webpage. If the sketch is created with a <div> element as the second parameter (Ex. new p5(sketch, container);), a loading indicator will appear at the center of the <div> container instead. I looked at the p5.js web editor and thought this would be useful in case you wanted to only embed the loading indictor onto the preview. Is there another approach to the loading indicators that you have in mind?
…ing indicator, "this" is referenced so that every p5 instance has their own loading indicator. Also changed the animation and style names to include "p5"
…the center of the webpage.
|
Hello! So after some discussion with @limzykenneth and @davepagurek, I'll go ahead and merge this but keep the issue open. I will also release a patch with this loading indication, so that it's available sooner. However I'll keep the parent issue open and add a comment there describing a followup to address the instance mode comments, which includes using a canvas-based animation rather than css. |
3 participants
Resolves #8922
Changes:
showLoadingIndicator()is called, a div element with a "Loading..." text will be created. WhenhideLoadingIndicator()is called, the div element will be removed from the DOM.showLoadingIndicator()andhideLoadingIndicator()are called before and afterawait context.setup(). The loading indicators will only appear while the canvas is loading thesetup()function in sketch.js.Screenshots of the change:
All tests, including loading.js, pass. The only exception is a typography which always failed, even before changes were made to the project.
PR Checklist
npm run lintpassesAlso worked with @joshin on this issue.