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

Review splash.js and related code #392

Closed
samreid opened this issue Dec 22, 2016 · 4 comments
Closed

Review splash.js and related code #392

samreid opened this issue Dec 22, 2016 · 4 comments

Comments

@samreid
Copy link
Member

samreid commented Dec 22, 2016

In #382 the splash screen was revised to support dynamically resizing.

From #382 (comment)

A developer needs to review the changes in the "splash-script" branches for joist, chipper and brand and do basic tests (run generate-development-html and test in requirejs, and test a built sim).

Please keep in mind that this solution must support Chrome => "Save As..." and iOS Reading Mode, each of which saves part of the DOM. You'll see code that supports this by checking for the existence of DOM elements before creating/adding them to the document.

In #382 (comment) @ariel-phet recommended @jonathanolson for the review.

EDIT: added the branch name

@jonathanolson
Copy link
Contributor

It wasn't immediately clear how the progress bar was updated (by the ID progressBarForeground, used in Joist).

I'd recommend references in both locations (splash.js and in Sim.js).

There seem to be other IDs that aren't used anywhere else, e.g. 'progressBarBackground', 'progressBar'.

Additionally, is there a place with documentation about the restrictions we need to follow for the "Save As" and Reading Mode? I don't want to break those in Scenery changes.

@samreid
Copy link
Member Author

samreid commented Dec 27, 2016

It wasn't immediately clear how the progress bar was updated (by the ID progressBarForeground, used in Joist).

I'd recommend references in both locations (splash.js and in Sim.js).

I added docs in the preceding commits.

There seem to be other IDs that aren't used anywhere else, e.g. 'progressBarBackground', 'progressBar'.

While developing this, I thought of removing the unused IDs but realized they may come in handy if someone is trying to understand the DOM (say, in the dev tools). Still, based on your point, I removed the IDs and tested that it works nicely without them.

Additionally, is there a place with documentation about the restrictions we need to follow for the "Save As" and Reading Mode? I don't want to break those in Scenery changes.

I don't have a thorough understanding of "Save As" and "Reading Mode", or know where they are documented. What I have observed are these workarounds in Sim.js:

    // check to see if the sim div already exists in the DOM under the body. This is the case for https://github.com/phetsims/scenery/issues/174 (iOS offline reading list)
    if ( document.getElementById( 'sim' ) && document.getElementById( 'sim' ).parentNode === document.body ) {
      document.body.removeChild( document.getElementById( 'sim' ) );
    }
            // Support iOS Reading Mode, which saves a DOM snapshot after the progressBarForeground has already been
            // removed from the document, see https://github.com/phetsims/joist/issues/389
            if ( document.getElementById( 'progressBarForeground' ) ) {

For instance, see phetsims/scenery#174 which I believe is our first sighting and most thorough record of Reading List related problems. You and I also spoke about the requirejs synchronousity problem @phet-steele discovered that was causing problems for "Save As" in phetsims/chipper#499 (comment)

@samreid samreid assigned jonathanolson and unassigned samreid Dec 27, 2016
@jonathanolson
Copy link
Contributor

Ok, looks good to me. Thanks!

@samreid
Copy link
Member Author

samreid commented Jan 5, 2017

Thanks @jonathanolson, I'll continue in #382

@samreid samreid closed this as completed Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants