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-story-shopping] Add productDescription to all templates and max length #37993

Merged
merged 3 commits into from
Apr 5, 2022

Conversation

processprocess
Copy link
Contributor

@processprocess processprocess commented Mar 31, 2022

productDescription is a required field.
This PR adds it to each example html template.

This PR also sets a max character length of 3000, and adds ellipses if it's over the limit.

Screen Shot 2022-04-01 at 9 46 52 AM

@amp-owners-bot
Copy link

amp-owners-bot bot commented Mar 31, 2022

Hey @gmajoulet! These files were changed:

extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-config.js

@mszylkowski
Copy link
Contributor

I wonder (depending on how this is displayed) if you want to test different lengths of text for this section. It might be helpful to have larger LoremIpsums in there so we know how it looks. Still approving

@processprocess processprocess changed the title [amp-story-shopping] Add productDescription to all templates [amp-story-shopping] Add productDescription to all templates and max length Apr 1, 2022
@processprocess
Copy link
Contributor Author

@jshamble Please be sure the validation for productDetails checks a minimum of 20 chars and max of 3,000 chars.

@processprocess processprocess merged commit c9b09d0 into ampproject:main Apr 5, 2022
@processprocess processprocess deleted the description branch April 5, 2022 18:47
@jshamble
Copy link
Contributor

jshamble commented Apr 5, 2022

@processprocess I am not sure if having a hard restriction would be beneficial given the discussion of other languages having different length requirements etc. See #37474 (comment)

@processprocess
Copy link
Contributor Author

@processprocess I am not sure if having a hard restriction would be beneficial given the discussion of other languages having different length requirements etc. See #37474 (comment)

Got it! Thank you for the pointer to the discussion!

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.

4 participants