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

📖 Bento: Adopt new formatting for amp-base-carousel documentation #32212

Merged
merged 7 commits into from
Jan 27, 2021

Conversation

caroqliu
Copy link
Contributor

Related to #28284, incorporating format changes from #31944

Copy link
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

This looks amazing!!! I was mistaken about executable code samples and Bento, so just one comment about updating the code sample and a qq about one of the actions. Otherwise LGTM!

Comment on lines 133 to 140
**goToSlide(index: number)**
Moves the carousel to the slide specified by the `index` argument.

```
api.goToSlide(0); // Advance to first slide.
api.goToSlide(length - 1); // Advance to last slide.
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Do they need to include index: in the argument? Or just the index number?

Copy link
Contributor Author

@caroqliu caroqliu Jan 26, 2021

Choose a reason for hiding this comment

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

Just the index. The index: number is to give the parameter a type, but let me specify this inline instead so it's less confusing, something like this:

Suggested change
**goToSlide(index: number)**
Moves the carousel to the slide specified by the `index` argument.
```
api.goToSlide(0); // Advance to first slide.
api.goToSlide(length - 1); // Advance to last slide.
```
**goToSlide(index)**
Args:
- `index`: number [required]
Moves the carousel to the slide specified by the `index` argument.
Note: `index` will be normalized to a number greater than or equal to `0` and less than the number of slides given.
\```
api.goToSlide(0); // Advance to first slide.
api.goToSlide(length - 1); // Advance to last slide.
\```

Copy link
Contributor

Choose a reason for hiding this comment

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

TypeScript-like notation here I think is the best in the long term. We are also likely to publish TypeScript type declarations (based on our x.type.js) in the future too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dvoytenko are you suggesting keep as is?

Comment on lines 133 to 140
**goToSlide(index: number)**
Moves the carousel to the slide specified by the `index` argument.

```
api.goToSlide(0); // Advance to first slide.
api.goToSlide(length - 1); // Advance to last slide.
```

Copy link
Contributor

Choose a reason for hiding this comment

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

TypeScript-like notation here I think is the best in the long term. We are also likely to publish TypeScript type declarations (based on our x.type.js) in the future too.

extensions/amp-base-carousel/amp-base-carousel.md Outdated Show resolved Hide resolved
layout="responsive"
width="3"
height="1"
heights="(min-width: 600px) calc(100% * 4 * 3 / 2), calc(100% * 3 * 3 / 2)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I think we should continue advising to use CSS for this. E.g.

<style>
  #carousel1 {
    height: calc(100% * 3 * 3 / 2);
  }
  @media (min-width: 600px) {
    #carousel1 {
      height: calc(100% * 4 * 3 / 2);
    }
  }
</style>

Of course, that'd split the media queries between the element and the attributes. But IMHO it's better to fix one part of it than both have two parts "broken". We can also communicate later that we are pursuing Web spec update to support attribute media queries as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, this section is moved verbatim from its former location in the file, and the code samples here were created with the intention of being used on valid AMP documents. Do you still recommend making the change? Or perhaps duplicating the section so there is one version for valid AMP documents (status quo) and one version with the suggested CSS for Bento documents?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's for valid AMP, then let's keep as is. Sorry for the confusion. But I think we'll be stumbling into these deltas for some time.

@caroqliu caroqliu merged commit b591d35 into ampproject:master Jan 27, 2021
PetrBlaha pushed a commit to PetrBlaha/amphtml that referenced this pull request Jan 28, 2021
…mpproject#32212)

* Documentation for base-carousel

* Rename with correct id

* Review comments

* Remove backticks from existent link

* Remove (optional)

* Remove TODO

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

Successfully merging this pull request may close these issues.

None yet

4 participants