Amphtml: Display a description in analytics-notification example

[micajuine-ho]

In examples/analytics-notifications.amp.html, let's add a description to let the reader know what is happening behind the scenes and what to expect in the developer console.

[ajitsinghkaler]

@micajuine-ho Hi, I would like to work on this but I need more details on this. This is also my first contribution I would really appreciate if you can guide me through it.

[micajuine-ho]

@ajitsinghkaler Hi Ajit. Happy to you guide you through this. Have you taken a look at the resources in the contributing folder? This is a good link that describes how to contribute code.

As for the issue, I imagine that adding some text in analytics-notifications.amp.html to describe what is happening would be sufficient. You can learn about what is happening with that page, specifically the amp-user-notification tag, here.

Let me know if you have any other questions.

[ajitsinghkaler]

After setup whenever I open analytics-notifications.amp.html I get the following error. Do, I need any additional setup for CORS on that page. I also receive a blank page on chrome. I don't know what is the fault.

[micajuine-ho]

What commands are you running?

[ajitsinghkaler]

I'm running gulp command in the amphtml folder. After installing Gulp-CLI, nvm, yarn

[micajuine-ho]

I'm not sure about the CORS error, but the other errors make sense (because they're fake endpoints).

As for why it displays a blank page, take a look at this documentation.

It seems like the webpage is using the amp-user-notification tag in the same way that is shown in Example 1. Try substituting it out for Example 2 :)

[noodles]

Is this the kind of thing you're looking for?

[micajuine-ho]

@noodles Perfect! Would you like to create a PR for this solution?

[noodles]

@ajitsinghkaler are you still in interested in this issue?

[ajitsinghkaler]

Yes, I am but I can't figure out the CORS error is it okay if I still create a PR using your content.

[noodles]

@ajitsinghkaler to get rid of the CORS error, I replaced the amp-user-notification in the demo with 'Example 2' from the documentation @micajuine-ho mentioned.

<amp-user-notification
      layout=nodisplay
      id="amp-user-notification6"
      data-persist-dismissal="false">
This notification should ALWAYS show on every page visit.
<a href="#learn-more">Learn more.</a>
<button on="tap:amp-user-notification6.dismiss">Dismiss</button>
</amp-user-notification>

After that, I just edited the text in the notification and added an explanation to the body of the page. Does that help?

[ajitsinghkaler]

So, should I generate the pull request or you are generating it because you solved it first?

[noodles]

@ajitsinghkaler you found the issue first so please go ahead and create your own PR.

[ajitsinghkaler]

Thanks @noodles

[ajitsinghkaler]

@micajuine-ho is there a problem in the first example if there is a problem then should I create a new issue for that.

[micajuine-ho]

@ajitsinghkaler I don't think there is a problem with example 1. Example 2 is just more straightforward and easier to understand :)

[ajitsinghkaler]

@micajuine-ho So should example 2 be the default example. What are your thoughts on it?

[micajuine-ho]

@ajitsinghkaler Example 1 will only display if the data-show-if-href gets a positive response from its endpoint. However since the endpoint in the example is fake, it will not show, which could be confusing.

Example 2 will always show, and so is less confusing. We should use example 2 instead of 1 to reduce confusion.

[ajitsinghkaler]

So should I create an issue for that

[micajuine-ho]

No, this is the place for this issue.

[ajitsinghkaler]

I was asking if we should create a new issue for that

[micajuine-ho]

You should not, you can just create a PR and then reference this issue in the PR.

[ajitsinghkaler]

Okay, thanks for your help