Skip to content
This repository has been archived by the owner on May 21, 2018. It is now read-only.

Added loading svg prior to init #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Nolski
Copy link
Contributor

@Nolski Nolski commented Aug 16, 2016

Resolves #70


loadingSpinner = document.createElement('img');
loadingSpinner.setAttribute('class', 'loading-spinner');
loadingSpinner.setAttribute('src', 'PopcornEditor/resources/ring-alt.svg');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sanity check here: is this the proper way we should read resources from the library outside of the iframe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm a bit confused. This is how I see it atm:

  • PopcornEditor loads an iframe
  • In the iframe is the actual editor, which includes PopcornEditor/resources/ring-alt.svg.
  • This code reaches into the src that's meant for the content inside the iframe to get an image.

If the stuff inside src is meant to be its own beast, I'd put this image outside in the context of PopcornEditor.js.

Otherwise, ¯_(ツ)_/¯ sure! This makes sense.

@traceypooh
Copy link
Collaborator

not sure on the outside an iframe question.
otherwise it looks good.
only thing that comes to mind is wondering if you / popcorn future would prefer the more compact and JSON-y (now or in the future) along the lines of:
jQuery('

').attr({'id','loading'}).css({width:"100%", display:"flex", ..})...

@benrito
Copy link
Collaborator

benrito commented Sep 20, 2016

@secretrobotron can you review and merge?

loadingDiv.style.height = '100%';
loadingDiv.style.backgroundColor = 'white';
loadingDiv.style.display = 'flex';
loadingDiv.style.justifyContent = 'center';
Copy link
Contributor

Choose a reason for hiding this comment

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

If it meshes with the architecture you're thinking here, I'd put this in index.html or some css file, just to be clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

index.html is just meant to be a an example though correct?

loadingSpinner.style.width = '30%';

loadingDiv.appendChild(loadingSpinner);
editor.appendChild(loadingDiv);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about putting this in index.html as above. Could probably go within the html there, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same goes for above. PopcornEditor.js can be included in anyone's web page so I assume that while they would use index.html as a reference, they shouldn't necessarily have to copy over the css and stuff in it.

@benrito
Copy link
Collaborator

benrito commented Sep 20, 2016

@Nolski can you have a look and let us know how to proceed? We want to deploy this before Monday—that's when we expect a relative surge of traffic until the election

@Nolski
Copy link
Contributor Author

Nolski commented Sep 23, 2016

Sure thing! I'll try to take a look tonight or tomorrow.

@Nolski
Copy link
Contributor Author

Nolski commented Dec 1, 2016

How's this looking? I pushed back on a couple of the comments but am happy to discuss either way

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants