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 wrong event name amp-carousel-next -> amp-carousel-prev #10640

Merged
merged 1 commit into from Aug 14, 2017

Conversation

ruborg
Copy link
Contributor

@ruborg ruborg commented Jul 26, 2017

No description provided.

@jridgewell
Copy link
Contributor

/cc @rsimha-amp: We have a failing link check again.

@rsimha
Copy link
Contributor

rsimha commented Jul 28, 2017

That's likely due to the fact that it's a relative link, or due to the page anchor. Try using a full link.

@mrjoro
Copy link
Member

mrjoro commented Aug 14, 2017

This is a pretty trivial change; any objection to just merging it?

@jridgewell
Copy link
Contributor

It'll fail new builds on master, till fixed.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

The issue stems from the fact that it's an incorrect link. You'll need to replace /extensions/amp-analytics/analytics-vars.md#fromslide with ../amp-analytics/analytics-vars.md#fromslide.

I just ran this locally, and the fix works. To test this out yourself, run gulp check-links --files extensions/amp-carousel/amp-carousel-analytics.md.

Edit: Found the real problem.

@rsimha
Copy link
Contributor

rsimha commented Aug 14, 2017

Test results before and after fixing the problematic link: https://gist.github.com/rsimha-amp/c63b519ab33ecbedd5d0540ba446afcb

@rsimha
Copy link
Contributor

rsimha commented Aug 14, 2017

@mrjoro, the link checker seems to have found a genuine case of an incorrect link. Fix based on comment above: #10640 (review)

@rsimha
Copy link
Contributor

rsimha commented Aug 14, 2017

Approving based on fix in #10906

@rsimha rsimha closed this Aug 14, 2017
@rsimha rsimha reopened this Aug 14, 2017
@mrjoro mrjoro merged commit 3146395 into ampproject:master Aug 14, 2017
@mrjoro
Copy link
Member

mrjoro commented Aug 14, 2017

Phew! It normally doesn't take this much to get a 4 character change in. :) That's for the fix @ruborg.

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

Successfully merging this pull request may close these issues.

None yet

5 participants