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

fix(ras-acc): respect social icons padding #1615

Merged

Conversation

chickenn00dle
Copy link
Contributor

@chickenn00dle chickenn00dle commented Aug 14, 2024

All Submissions:

Changes proposed in this Pull Request:

Closes https://app.asana.com/0/1207817176293825/1207990881852220/f

This PR addresses an issue where the renderer was not not respecting the social icons padding set in the editor. This also tweaks the default icons padding to 8px (from 7px) to be in-line with the spacing set in RAS-ACC transactional emails:

Editor:
Screenshot 2024-08-14 at 14 58 26

Email:
Screenshot 2024-08-14 at 14 58 34

How to test the changes in this Pull Request:

To test this PR, be sure to also checkout the following plugin PR: Automattic/newspack-plugin#3339

See Automattic/newspack-plugin#3339 for testing RAS-ACC specific behavior

  1. Add social icons to any newsletter
  2. Preview and send the newsletter
  3. Confirm the behavior matches what is on trunk

Note that social icons in newsletters is currently a bit funky on trunk. This PR does not fix this and only focuses on changes for RAS-ACC.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@chickenn00dle chickenn00dle requested a review from a team as a code owner August 14, 2024 19:00
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 19.67%. Comparing base (8257b67) to head (15045ff).
Report is 1 commits behind head on epic/ras-acc.

Additional details and impacted files
@@                Coverage Diff                 @@
##             epic/ras-acc    #1615      +/-   ##
==================================================
+ Coverage           19.63%   19.67%   +0.03%     
- Complexity           2372     2378       +6     
==================================================
  Files                  45       45              
  Lines                8926     8930       +4     
==================================================
+ Hits                 1753     1757       +4     
  Misses               7173     7173              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

This looks good! I couldn't spot any noticeable difference between the before and after in newsletters.

@laurelfulford laurelfulford self-requested a review August 15, 2024 00:48
@chickenn00dle chickenn00dle merged commit 31f0daf into epic/ras-acc Aug 15, 2024
11 checks passed
@chickenn00dle chickenn00dle deleted the fix/transaction-emails-social-icons-alignment branch August 15, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants