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

update ui playground middleware to latest 1.7.19 from 1.7.10 #582

Merged
merged 3 commits into from Feb 12, 2019

Conversation

Projects
None yet
3 participants
@drowhunter
Copy link
Contributor

drowhunter commented Feb 4, 2019

Implements #563

@michaelstaib michaelstaib requested a review from rstaib Feb 4, 2019

@michaelstaib michaelstaib added this to the 0.8.0 milestone Feb 4, 2019

@rstaib
Copy link
Member

rstaib left a comment

Hi @drowhunter ,

I will check out your pull request soon.

@rstaib
Copy link
Member

rstaib left a comment

Thank you for updating our Playground middleware to the latest version. I have added some comment to the index.html file.

@@ -520,15 +520,15 @@
</div>
</div>

<div id="root"></div>
<div id="root" />

This comment has been minimized.

@rstaib

rstaib Feb 11, 2019

Member

Self closing elements like <div /> have negative side-effects in HTML5. In this case the following script-tag becomes a child of div. Please revert your changes in this case.

This comment has been minimized.

@drowhunter

drowhunter Feb 12, 2019

Author Contributor

fixed

<script type="text/javascript">
window.addEventListener('load', function (event) {
var loadingWrapper = document.getElementById('loading-wrapper');
const loadingWrapper = document.getElementById('loading-wrapper');

This comment has been minimized.

@rstaib

rstaib Feb 11, 2019

Member

We should still use var here because it's ES5 compliant. For example, if someone still uses IE10 for whatever reason, this would result in an error.

This comment has been minimized.

@drowhunter

drowhunter Feb 12, 2019

Author Contributor

fixed

loadingWrapper.classList.add('fadeOut');
var root = document.getElementById('root');
const root = document.getElementById('root');

This comment has been minimized.

@rstaib

rstaib Feb 11, 2019

Member

Same here; not ES5 compliant.

This comment has been minimized.

@drowhunter

drowhunter Feb 12, 2019

Author Contributor

fixed

@drowhunter

This comment has been minimized.

Copy link
Contributor Author

drowhunter commented Feb 12, 2019

ok i have fixed the above issues and pushed to my fork

@rstaib

rstaib approved these changes Feb 12, 2019

@rstaib rstaib merged commit 4b22f09 into ChilliCream:master Feb 12, 2019

3 checks passed

Better Code Hub ✅ Better Code Hub approves this code
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rstaib

This comment has been minimized.

Copy link
Member

rstaib commented Feb 12, 2019

landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment