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

Fix #736: Drop xorigin.js and dependencies.js to reduce startup time #741

Closed
wants to merge 8 commits into from

Conversation

Th30
Copy link

@Th30 Th30 commented Apr 21, 2017

Should be working fine. I had to use window.setTimeout to avoid the following error:

image

Let me know if I'm missing comments or if there are any problems.

#736

Edit:

slimmed down the code a bit. I could get rid of str, but kept it because it's easier on the eyes:)

src/main.js Outdated
@@ -120,6 +120,22 @@ if ('serviceWorker' in window.navigator) {
});
}

window.setTimeout(function () {
Copy link

Choose a reason for hiding this comment

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

I want to avoid this. A better way for us to do this is to take the HTML you have below, and insert it into a <div id="iframe-warning">..</div> inside the spinner container, here. You can add some CSS to the inline style block to hide this div by default. Then, when your JS runs, if we enter the if block you have on 129`, we can show the error (e.g., add/change the CSS class on the div) and skip loading Bramble.

Copy link
Author

Choose a reason for hiding this comment

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

I'll be home tonight at 8 I'll work on this tonight and get it changes asap!

Copy link
Author

@Th30 Th30 Apr 24, 2017

Choose a reason for hiding this comment

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

@humphd some progress
image

Edit 1 Ok fixed
image

Copy link
Author

@Th30 Th30 Apr 24, 2017

Choose a reason for hiding this comment

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

@humphd ok everything should be fixed, and pushed let me know if there are anymore changes. :) I tested again and working across all 3 browsers

@Th30
Copy link
Author

Th30 commented Apr 27, 2017

@humphd any news? :)

Copy link

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is looking pretty good. A few things to improve it, and we should be good to go.

NOTE: I'll not be able to return to this for over a week.

src/index.html Outdated
@@ -57,6 +57,9 @@
*/
outline: 1px solid transparent;
}
#iframe-warning {
visibility: hidden;
}
Copy link

Choose a reason for hiding this comment

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

Let's add padding: 20px; to this too, so it isn't so tight to the edge of the window.

src/main.js Outdated
);
}
}, 1000);
}
Copy link

Choose a reason for hiding this comment

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

Probably we should throw this remaining code in an else block, since it's not worth doing if the first if happens.

Copy link
Author

@Th30 Th30 Apr 27, 2017

Choose a reason for hiding this comment

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

I'm not sure what you mean? isn't the if statement required to check to see if it's within iFrame? I don't see us doing it anywhere else? @humphd

Edit: ah ok I get what you mean:)

@Th30
Copy link
Author

Th30 commented Apr 27, 2017

@humphd I hope I understood what you meant, I basically moved the if statement to the top, then added an else for the rest of the code. This way it only executes if within iframe.

@Th30
Copy link
Author

Th30 commented May 11, 2017

@humphd hey any news on this bug? :)

@humphd
Copy link

humphd commented May 11, 2017

It's on our radar, but not part of what we're shipping next week. After that I'll circle back.

@Th30
Copy link
Author

Th30 commented May 11, 2017

Alright keep me posted.

@humphd
Copy link

humphd commented May 31, 2017

I've finished this in #779.

@humphd humphd closed this May 31, 2017
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.

None yet

2 participants