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][amp-date-countdown] Change attribute name of count-up #32201

Merged
merged 4 commits into from
Jan 27, 2021

Conversation

krdwan
Copy link
Contributor

@krdwan krdwan commented Jan 26, 2021

Change the data-count-up attribute to be named count-up.

This matches the rest of the attributes and unlikely to have collision with native attributes since there is a dash in count-up.

Related to: #32047
#32047 (comment)

@krdwan krdwan marked this pull request as ready for review January 26, 2021 16:06
@krdwan
Copy link
Contributor Author

krdwan commented Jan 26, 2021

@CrystalOnScript Adding you for changes to documentation :), in case there's anything that I have not worded well!

@@ -95,7 +95,7 @@ This table provides examples of formatted values specified in a Mustache templat

### Migrating from 0.1

The experimental `1.0` version of `amp-date-countdown` does not support the `data-count-up` attribute. This means the `1.0` component is unable to count in the opposite direction.
The experimental `1.0` version of `amp-date-countdown` uses the attribute name `count-up` instead of `data-count-up` as in `0.1` to support the `count up` feature. See the `count-up` section under `Attributes` below for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "count up" feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated!

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.

LGTM with one suggestion

Co-authored-by: CrystalOnScript <crystallambert@google.com>
@krdwan krdwan merged commit a497941 into ampproject:master Jan 27, 2021
@krdwan
Copy link
Contributor Author

krdwan commented Jan 27, 2021

@caroqliu Merging now so I can use a fresh master for Bento docs PR. If you have any outstanding comments please comment here: #32236

Copy link
Contributor

@caroqliu caroqliu left a comment

Choose a reason for hiding this comment

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

Sorry I missed this! LGTM!

PetrBlaha pushed a commit to PetrBlaha/amphtml that referenced this pull request Jan 28, 2021
…roject#32201)

* Change attribute name of count-up

* Add doc updates for date-countdown

* Add quotes to count up in docs

* Update extensions/amp-date-countdown/amp-date-countdown.md

Co-authored-by: CrystalOnScript <crystallambert@google.com>

Co-authored-by: CrystalOnScript <crystallambert@google.com>
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

5 participants