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

demo(text-field): Move script into head #1636

Conversation

anton-kachurin
Copy link
Contributor

This removes the console error message caused by scripts trying to use the global variable mdc before it was created:
text-field.html:184 Uncaught ReferenceError: mdc is not defined

@codecov-io
Copy link

codecov-io commented Nov 23, 2017

Codecov Report

Merging #1636 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1636   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files          72       72           
  Lines        3439     3439           
  Branches      420      420           
=======================================
  Hits         3426     3426           
  Misses         13       13

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 875e393...f5722d6. Read the comment docs.

Copy link
Contributor

@acdvorak acdvorak left a comment

Choose a reason for hiding this comment

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

Could you possibly create an issue for this?

I've never encountered that error, so I'd be curious to know:

  1. How often you see the error
  2. Which version of MDC-Web you're using
  3. Which devices, OS versions, browser versions, etc. it occurs in
  4. Whether it occurs more frequently when you're on a slow network or a slow device
  5. Whether you see the same error on other demo pages
  6. What the line number and stack trace are
  7. What the repro steps are

(Please include the above in the issue.)

Presumably this kind of error is not specific to text field, and could just as easily happen to any other demo. If it repros reliably, we would want to apply a fix to all of our demo pages.

Also, I'm not sure that moving the <script> tag up into <head> is the right fix. It would be good to know why the error is happening in the first place. All of our <script> tags are blocking, synchronous, and evaluated sequentially, so unless Webpack is doing some funky setTimeout/requestAnimationFrame nonsense, I don't see why the error should even be possible.

@acdvorak acdvorak self-assigned this Nov 30, 2017
@lynnmercier
Copy link
Contributor

I filed an issue, #1808, because I was able to reproduce this quite quickly on our main demo site.

But it we should not merge this PR until we know what the root cause of the issue is. @anton-kachurin do you know why this is happening in the first place?

@acdvorak
Copy link
Contributor

Should be fixed by #1919 and #1920. Feel free to reopen if you still see the issue. Thanks! 😀

@acdvorak acdvorak closed this Jan 22, 2018
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.

4 participants