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

Cleaning up promise-from-getHeadStylesheets test page #115

Merged
merged 1 commit into from Apr 27, 2016

Conversation

roblg
Copy link
Member

@roblg roblg commented Apr 27, 2016

Cleaning up getHeadStylesheets with promises test page.

@gigabo encouraged me to be cute, so I added a date endpoint for test pages that echoes CSS back.

@roblg
Copy link
Member Author

roblg commented Apr 27, 2016

BTW, I noticed some weird behavior that I filed as #116. I didn't have time to dig into it though.

<div>If first.css shows up before second.css in the body of this page, all is well.</div>,
<div>
<h2>If the background is GREEN at page load, all is well!</h2>
<div>If it flashes red first, all is _not_ well.</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it actually the page winding up red that we're looking out for? In other words out of order CSS includes?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be, but for #116. ClientCssHelper getting confused + the fact that it actually waits until all promises are resolved means that everything currently ends up correct at the end (after a 1s pause w/ a red background)

Copy link
Member Author

Choose a reason for hiding this comment

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

Er. That is, everything works in this PR. You have to hand-modify renderMiddleware to the not-chained behavior in order to see the flash of red

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Thanks!

@gigabo
Copy link
Contributor

gigabo commented Apr 27, 2016

@gigabo encouraged me to be cute

😘

This is great! Plus you found a bug while you were working on it!

@roblg roblg merged commit 16de869 into redfin:master Apr 27, 2016
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