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

AO3-6548 Use pseud helpers in comment emails and fix kudos image display #4691

Merged
merged 10 commits into from
Dec 22, 2023

Conversation

EchoEkhi
Copy link
Contributor

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-6548

Purpose

Fix email style in AO3-6548

Testing Instructions

See JIRA ticket

Credit

EchoEkhi (He/him)

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

Small note: This sets the red color on both the <b> tag and the <a> tag. But I don't think this matters, batch_subscription_notification.html.erb does it too. If it does matter then this could use tag.strong instead of style_bold.

Comment on lines 200 to 202
background: url("/images/imageset.png") no-repeat -111px -580px;
width: 55px;
height: 54px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
background: url("/images/imageset.png") no-repeat -111px -580px;
width: 55px;
height: 54px;
background: url("/images/imageset.png") no-repeat -110px -580px;
width: 55px;
height: 55px;

This matches more what is in the png.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

It shows a bit of the adjacent images when viewed in Firefox if I change it to round numbers. Works fine on chrome though.

@Bilka2
Copy link
Contributor

Bilka2 commented Dec 15, 2023

I can confirm that the ::before trick fixes the kudos image display. However, it's now positioned lower than before.
This PR on the left, staging in the middle and production on the right:
image

Furthermore, the text flows differently if it's long (= many kudos by users):
Screenshot_20231215_110334

I don't know whether that's okay. Also, the min-height change in #4686 moved the comment box up (visible in staging + dev in the first screenshot). Again, no idea if that is okay. But leaving my screenshots here in case it helps someone else review.

@@ -186,7 +186,7 @@ def commenter_pseud_or_name_link(comment)
elsif comment.by_anonymous_creator?
style_bold(t("roles.anonymous_creator"))
else
style_link(comment.comment_owner_name, polymorphic_url(comment.comment_owner, only_path: false))
style_bold(style_link(comment.comment_owner_name, polymorphic_url(comment.comment_owner, only_path: false)))
Copy link
Member

Choose a reason for hiding this comment

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

@sarken pointed out to me that we have a helper that can help here:

def style_pseud_link(pseud)
style_link("<img src=\"" + root_url + "favicon.ico\" style=\"border:none;display:inline-block;font-weight:bold;height:16px;padding-right:3px;vertical-align:-3px;width:16px;\">" +
pseud.byline, user_pseud_url(pseud.user, pseud))
end
def text_pseud(pseud)
pseud.byline + " (#{user_pseud_url(pseud.user, pseud)})"
end
(it does include the kudos icon, but we're OK with that as another way to distinguish registered users from guests). It seems to play nicely with style_bold but we're fine as long as one of the icon or bold is there

Copy link
Collaborator

@sarken sarken left a comment

Choose a reason for hiding this comment

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

Oh, wait, my mistake! commenter_pseud_or_name_text also needs to be updated to use text_pseud (the plain text equivalent of style_pseud_link).

@sarken sarken changed the title AO3-6548 Fix bold style in email AO3-6548 Use pseud helpers in comment emails and fix kudos image display Dec 17, 2023
@sarken
Copy link
Collaborator

sarken commented Dec 17, 2023

Bilka can't access GitHub right now, but he pointed out we forgot some mailers 🤦

In app/views/admin_mailer/, the following mailers need to be updated to use commenter_pseud_or_name_link and commenter_pseud_or_name_text:

  • comment_notification.html.erb
  • comment_notification.text.erb
  • edited_comment_notification.html.erb
  • edited_comment_notification.text.erb

@sarken sarken merged commit cb58df0 into otwcode:master Dec 22, 2023
26 checks passed
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.

4 participants