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

Make adding a CTA button with text more intuitive #1562

Closed
1 task done
alcurrie opened this issue Oct 30, 2018 · 8 comments
Closed
1 task done

Make adding a CTA button with text more intuitive #1562

alcurrie opened this issue Oct 30, 2018 · 8 comments
Assignees
Labels
Enhancement New feature or improvement of an existing one
Milestone

Comments

@alcurrie
Copy link

alcurrie commented Oct 30, 2018

As a AMP stories author, I want to the process for adding a CTA button with text to be more intuitive so that i can easily add a CTA to my AMP story.

  • AC 1: Make adding a CTA button with text more intuitive.
    - [ x] AC 2: Resolve the issue identified here: After adding a CTA button, it is difficult to figure out how to add the button text. There is an interface that allows the user to add a URL for the button, but the interface to edit the text of the CTA button requires the user to click on the B (for Bold) or I (for Italics) to access the interface to add a button label. https://cl.ly/640fd0fc03ea
    - [x] AC 3: Consider modifying the CTA layer block to add a block template which populates a newly-inserted block with a button block to begin with, instead of a paragraph.

Relates to: #1560

@alcurrie
Copy link
Author

alcurrie commented Nov 1, 2018

@jwold / @mehigh - @miina - flagged that the issue documented in AC 2, https://cl.ly/640fd0fc03ea, is no longer repeatable, likely due to a Gutenberg update. See screencast: https://drive.google.com/file/d/1T_uXOvyIKE8h5Tzvdud2cHhm3qfeogoN/view?usp=sharing
However, the interface is still not intuitive, so the requirement in AC-3 or something that resolves that that same issue is still needed I think.

@jwold
Copy link
Collaborator

jwold commented Nov 1, 2018

AC 3 is the only issue actually on this ticket, so therefore dev will move forward to followup on it, and if needed, will reach out for UX feedback, but it's not likely at this point.

@jwold jwold removed the UX label Nov 1, 2018
@miina miina added the BE label Nov 1, 2018
@miina miina self-assigned this Nov 12, 2018
@miina miina removed their assignment Nov 12, 2018
@jacobschweitzer jacobschweitzer self-assigned this Nov 21, 2018
@jacobschweitzer
Copy link
Contributor

@miina Could you please review this PR for me? Thanks!

I tried to add a RichText template but it was giving an error that attributes was empty though it seemed to work fine with existing blocks so I didn't catch it right away. I also tried with core/paragraph as the template but it was acting funny as well. Is there a place where <InnerBlocks are defined for the CTA layer that I missed or am I in conflict with a Gutenberg default which is added?

@miina
Copy link
Contributor

miina commented Nov 22, 2018

@jacobschweitzer Just added some comments.

Saw your note just now about adding a RichText. Actually I think we were supposed to add core/button (Button block) template -- could you try with that to see if this works? If there's an issue with Gutenberg that doesn't allow doing that at this moment then we should log this for the next iteration of AMP Stories and can probably leave this AC out for now. Would also be good to check in github if there's a similar issue for using templates + allowedBlocks together -- perhaps that's the case here.

@jacobschweitzer
Copy link
Contributor

@miina I've updated it with core/button in the PR. I think I just misread the AC. Please have a look at the PR in relation to the other two issues there (blurring and CTA on first page).

@miina miina assigned alcurrie and unassigned jacobschweitzer and miina Nov 22, 2018
@miina
Copy link
Contributor

miina commented Nov 22, 2018

Closing this this got fixed within #1636 .

@miina miina closed this as completed Nov 22, 2018
@alcurrie
Copy link
Author

The core functionality is confirmed working. @csossi - can you review this as part of your overall QA review.

@alcurrie alcurrie assigned alcurrie and unassigned alcurrie and csossi Nov 30, 2018
@alcurrie
Copy link
Author

alcurrie commented Dec 3, 2018

Confirmed the interface to add a CTA button is now much simpler and easier to add to an AMP story https://cl.ly/5dc8d80adcd6

@westonruter westonruter added this to the v1.2 milestone May 21, 2019
@swissspidy swissspidy added Enhancement New feature or improvement of an existing one and removed AMP-Stories-Extension labels Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one
Projects
None yet
Development

No branches or pull requests

7 participants