Skip to content

[amp-story-shopping] Add productDescription to all templates and max length#37993

Merged
processprocess merged 3 commits intoampproject:mainfrom
processprocess:description
Apr 5, 2022
Merged

[amp-story-shopping] Add productDescription to all templates and max length#37993
processprocess merged 3 commits intoampproject:mainfrom
processprocess:description

Conversation

@processprocess
Copy link
Copy Markdown
Contributor

@processprocess processprocess commented Mar 31, 2022 •
edited
Loading

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

@processprocess processprocess self-assigned this Mar 31, 2022
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Mar 31, 2022 •
edited
Loading

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
Copy Markdown
Contributor

mszylkowski commented Mar 31, 2022

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
Copy Markdown
Contributor Author

processprocess commented Apr 1, 2022

@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
Copy Markdown
Contributor

jshamble commented Apr 5, 2022 •
edited
Loading

@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
Copy Markdown
Contributor Author

processprocess 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)

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

@jshamble jshamble mentioned this pull request Apr 8, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mszylkowski mszylkowski mszylkowski approved these changes

@jshamble jshamble Awaiting requested review from jshamble

Assignees

@processprocess processprocess

Labels

P1: High Priority PR use: In Beta / Experimental PR use: In LTS PR use: In Stable WG: stories

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@processprocess @mszylkowski @jshamble @ampprojectbot