-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
4845 pray and pay revisions pt. 3 #4912
4845 pray and pay revisions pt. 3 #4912
Conversation
for more information, see https://pre-commit.ci
…ne/courtlistener into 4845-pray-and-pay-revisions
…ne/courtlistener into 4845-pray-and-pay-revisions
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.
Not sure if this is ready, but I went ahead and did a quick review. Hopefully helpful. Thank you as always!
…ne/courtlistener into 4845-pray-and-pay-revisions
I'm continuing to break PRs into chunks and this should be ready 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.
Great! Merging!
<li><a href="{% url "tag_list" username=user.username %}" | ||
tabindex="205"><i class="fa fa-tags gray fa-fw"></i> Tags</a></li> | ||
<li><a href="{% url 'user_prayers' user.username %}" | ||
tabindex="206"><i class="fa gray fa-fw"></i> Prayers</a></li><!--need to fix class--> |
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.
Oops, just noticed this isn't behind the flag, and that it has a todo for the icon. It's probably fine at this point if the user prayers page is available to folks, but we should fix the icon quickly. Did you have a plan for this, @v-anne?
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.
I'll submit a hotfix to put it behind the flag for the time being.
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.
I kind of like this one:
https://fontawesome.com/v5/icons/hand-holding-heart?f=classic&s=solid
But it's not supported by v4 of Font Awesome. Upgrading doesn't make sense since we're redoing the website anyway, so probably something else makes more sense, but I haven't found one I like. We could hide it for now, in any case.
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.
https://fontawesome.com/icons/hands-praying?f=classic&s=solid
I came across this one too, but it may have the same issue.
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.
It does. I think it also is kind if a bad icon!
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.
I kind of like this one:
https://fontawesome.com/v5/icons/hand-holding-heart?f=classic&s=solid
But it's not supported by v4 of Font Awesome.
Hey @mlissner and @v-anne sorry I'm a bit late to the conversation. I don't think an upgrade is necessary just for this one icon.
I checked the link and noticed there's an SVG version of the icon available. We can use that if we really like it.
Let me know what you think!
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.
I think that looks good!
This is part 3 of the revisions to pray-and-pay outlined in #4845.
Included:
get_lifetime_prayer_stats