-
Notifications
You must be signed in to change notification settings - Fork 169
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
MWPW-144549 CTA alignment for Text, Icon, and Media Blocks #2209
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2209 +/- ##
==========================================
+ Coverage 95.64% 95.65% +0.01%
==========================================
Files 166 166
Lines 43871 43896 +25
==========================================
+ Hits 41961 41990 +29
+ Misses 1910 1906 -4 ☔ View full report in Codecov by Sentry. |
.text-block.icon-inline .foreground [class^="body-"], | ||
.text-block.icon-inline .foreground .cta-container { |
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.
This is the only update from the previous PR #2098
@JasonHowellSlavin @Ruchika4 @ryanmparrish @elan-tbx I'd appreciate your re-review on this. The only new code is in text.css, lines 170-171. You've previously reviewed this PR (here: #2098) but there was an issue found during stage testing (here: #2203 (comment)). It was found in the UAR quiz and applies to all Text block inline-icon variants. I've added that variant to the text-alignment-all page and you can also see it in the fragment from the UAR quiz on CC: https://main--cc--adobecom.hlx.live/cc-shared/fragments/uar/marquee/marquee-plan/cci-cct?milolibs=methomas-text-alignment Thanks! |
This PR is currently in the |
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.
Looks good 👍 I just added a few possible improvements
const ctaArea = medias[0].querySelector('.cta-container .action-area'); | ||
expect(ctaArea).to.exist; |
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.
Nit: We can turn this into a one liner by not caching ctaArea.
You can just do: expect(medias[0].querySelector('.cta-container .action-area')).to.exist;
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.
In general I agree but I think it improves readability of the tests so I'll keep them as is.
Resolves: MWPW-144549
Test URLs:
Before: https://main--milo--adobecom.hlx.page/drafts/methomas/text-alignment
After: https://methomas-text-alignment--milo--adobecom.hlx.page/drafts/methomas/text-alignment?martech=off
Bacom:
Kitchen sink:
Edge cases and regression:
UAR: