-
Notifications
You must be signed in to change notification settings - Fork 21
Added new fields and styling for Campaign pages to allow for CTA-first layouts #1756
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for coming up with a solution with better A11Y support. In addition to the inline comments I left, I also have a question regarding if a new form header is needed. Having two big headers on the same viewport seems to make the page a bit busy. Maybe this is something we should check with the design team to get styling suggestions.
Co-authored-by: Mavis Ou <mmmavis@users.noreply.github.com>
@mmmavis Happy to pull out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mtdenton thanks for the tag!
From a backend standpoint, this looks good to me! I have also tested it out on my local machine and can confirm that its working as expected.
I see that Mavis has left some design related comments, so I am going to leave my review as a comment, and will leave the approval to Mavis once the design stuff has been taken care of.
Also, in regards to this comment:
Thanks for coming up with a solution with better A11Y support. In addition to the inline comments I left, I also have a question regarding if a new form header is needed. Having two big headers on the same viewport seems to make the page a bit busy. Maybe this is something we should check with the design team to get styling suggestions.
@mmmavis Happy to pull out
donate_header
, just need to find another solution for getting theintro_header
above the CTA on mobile that doesn't duplicate thath2
in the HTML. I'm thinking the best route is to pull it out into its own row, so I'm going to play around with that a bit.
I agree that it would be best to have it on its own row so we only need to render the h2 once.
However, if that ends up being a lot of trouble, the landing page achieves this through the use of two elements and the classes --mobile and --desktop, with CSS hiding them at the appropriate breakpoints here
Thank you @mtdenton for making the updates. One last thing - I noticed that when cta_first is off, we see two headers on narrow screens. Once that's fixed, this PR is good to go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohooooo! Thanks @mtdenton 🎉
Closes #1755
This adds a few things to campaign pages:
cta_first
boolean that determines the order of the markupcta_first
is checked, it retains the old layout style on tablet sizes and abovecampaign_page.html
have now been split out into fragments to increase readabilityChecklist
Remove unnecessary checks
Tests
Changes in Models:
Documentation: