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

Amp-font: Unhandled Promise Rejection #1095

Closed
NataliaLKB opened this issue Dec 8, 2015 · 13 comments
Closed

Amp-font: Unhandled Promise Rejection #1095

NataliaLKB opened this issue Dec 8, 2015 · 13 comments
Assignees

Comments

@NataliaLKB
Copy link
Contributor

When trying out amp-font, I receive an error:

image

The font works fine when used regularly (just usingfont-face).

Is this an error you could help me with? The test page is here

@cramforce
Copy link
Member

Thanks for the report! Assigning to @sriramkrish85

@camelburrito
Copy link
Contributor

@NataliaLKB , You are seeing this error because the font takes too long to download (default is 3s), if you want you can set a timeout attribute > 3000 and see if it loads.

I will send a PR soon that will have an warning (rather than an error) that is easy to read.

@cramforce
Copy link
Member

Yes, making this a warning sounds good. And lets spell out exactly what
happened.

On Tue, Dec 8, 2015 at 7:47 PM Sriram Krishnan notifications@github.com
wrote:

@NataliaLKB https://github.com/NataliaLKB , You are seeing this error
because the font takes too long to download (default is 3s), if you want
you can set a timeout attribute > 3000 and see if it loads.

I will send a PR soon that will have an warning (rather than an error)
that is easy to read.


Reply to this email directly or view it on GitHub
#1095 (comment)
.

camelburrito added a commit to camelburrito/amphtml that referenced this issue Dec 9, 2015
camelburrito pushed a commit that referenced this issue Dec 9, 2015
Amp-font: Un-handled Promise Rejection #1095
@camelburrito
Copy link
Contributor

@NataliaLKB - Made a console.warn - to give some useful message. Let me know if you are able to get the font to load after increasing your timeout. Closing this issue for now. Feel free to re-open if required.

@NataliaLKB
Copy link
Contributor Author

@sriramkrish85 thanks so much for taking a look.

The first thing I tried is increasing the timeout to 10s (10000). Loading the font through font-face seems to be instantaneous, certainly not 10s. Unfortunately it still doesn't work. For good measure I also tried it with 100s and no joy.

Can you think of anything else to try, or that I may have done wrong?

@camelburrito
Copy link
Contributor

@NataliaLKB , you will have to use the

on-error-remove-class
on-error-add-class
on-load-remove-class
on-load-add-class

to be able to use amp-font efficiently

You can take a look at examples/font.amp.html for usage and give it another shot, or ping me a gist of what you are trying and i would be happy to take a look.

@NataliaLKB
Copy link
Contributor Author

@sriramkrish85 currently on the page referenced above I have:

<amp-font layout="nodisplay"
    font-family="Guardian Egyptian Web"
    timeout="3000"
    on-load-add-class="guardian-egyptian-loaded"
    on-load-remove-class="guardian-egyptian-loading"
    on-error-remove-class="guardian-egyptian-loading"
    on-error-add-class="guardian-egyptian-missing">
 </amp-font>

Locally I have tried it with 100s

camelburrito added a commit to camelburrito/amphtml that referenced this issue Dec 9, 2015
camelburrito pushed a commit that referenced this issue Dec 9, 2015
camelburrito added a commit to camelburrito/amphtml that referenced this issue Dec 9, 2015
camelburrito pushed a commit that referenced this issue Dec 9, 2015
Bug fix #1095 - fix font-size for comparison.
@camelburrito
Copy link
Contributor

@NataliaLKB , thanks for the link to help debug the issue. I have pushed a fix. You should be good now (I also saw that this was already working on mobile safari without my fixes. My fixes would make this more stable.)

@erwinmombay
Copy link
Member

we need to do a release btw if we want them to receive the fix.

@erwinmombay
Copy link
Member

@NataliaLKB latest release should be out within the hour https://github.com/ampproject/amphtml/releases

@NataliaLKB
Copy link
Contributor Author

That is great! Thank you so much!
On Dec 9, 2015 9:23 PM, "erwin mombay" notifications@github.com wrote:

@NataliaLKB https://github.com/NataliaLKB latest release should be out
within the hour https://github.com/ampproject/amphtml/releases


Reply to this email directly or view it on GitHub
#1095 (comment)
.

@NataliaLKB
Copy link
Contributor Author

Just tested it out, and it works great now.

Thank you 😄

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

No branches or pull requests

4 participants