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

✨Allow custom text in ASAA CTA #25523

Merged
merged 10 commits into from Nov 18, 2019
Merged

✨Allow custom text in ASAA CTA #25523

merged 10 commits into from Nov 18, 2019

Conversation

calebcordry
Copy link
Member

Previously we only allowed CTA text from predefined choices, this allows ads to have custom text.

The new behavior is that the button will grow horizontally to fit the new variable text length. If the text is too long to fit within the design spec, the ad will be discarded.

@calebcordry
Copy link
Member Author

@lannka @powerivq friendly ping :) cc/ @katrinivini

Copy link
Contributor

@powerivq powerivq left a comment

Choose a reason for hiding this comment

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

w/ comments

this.measurer_ = this.doc_.createElement('div');

/** @private {number} Fixed button height from design spec. */
this.maxHeight_ = 32;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always fixed? Should it defined as a constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to const

maxFontSize
) {
maxFontSize++;
// Binomial search for the best font size.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be
s/Binomial/Binary/

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed with the fit logic changes.

* @param {number} maxFontSize
* @return {number}
*/
function calculateFontSize(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for future proofing? The current range is very small at 12-14

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, refactored to start at 14 and work down

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit detailed, but I think a for loop would be more readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to for loop.

Copy link
Contributor

@powerivq powerivq left a comment

Choose a reason for hiding this comment

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

lgtm w/ comment.

* @param {number} maxFontSize
* @return {number}
*/
function calculateFontSize(
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit detailed, but I think a for loop would be more readable.

@calebcordry calebcordry merged commit 97ed0db into ampproject:master Nov 18, 2019
@calebcordry calebcordry deleted the button-fit branch November 18, 2019 21:53
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
* works but not handling async

* async all the things

* fix types

* fix auto ad tests

* fix page tests

* new tests

* fix e2e

* remove binary search

* move max height

* use for loop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants