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-5421 Adds line-height to userstuff list items #4904

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anechol
Copy link

@anechol anechol commented Aug 24, 2024

Pull Request Checklist

Issue

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

Purpose

Adds line-height to list item tags within .userstuff class.

Testing Instructions

See Jira ticket

Credit

anechol (she/her)

@sarken
Copy link
Member

sarken commented Aug 25, 2024

Hi, anechol!

Thank you so much for this pull request. Someone will be along to review it soon.

I've updated the Jira issue status to In Review, so no one will mistakenly create a duplicate pull request. If you'd like the ability to comment on, assign, and transition issues in the future, you're welcome to create a Jira account! You can just reply here with the account name and we'll set up the permissions for you. (It makes things a bit easier for us on the organizational side if the Full Name on your Jira account either closely matches the name you'd like us to credit in the release notes or includes it in parentheses, e.g. "Nickname (PREFERRED NAME).")

Thanks again for contributing! If you have any questions, you can contact us at [email protected].

Copy link
Member

@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.

Thanks for looking into this!

I noticed this doesn't address the issue with text following block quotes, but when I tried to reproduce it using the steps outlined on Jira, the text was wrapped in paragraph tags 🎉 So that's probably not a huge issue going forward, although it will probably still be present in older works.

That being said, there's at least one other place this issue can occur: in dt tags. (I can update the issue to mention this and remove the block quote information.)

I'm wondering if, rather than trying to fix all the individual problem areas, it would make sense to apply the fix to .userstuff itself. I was a little leery that it might affect the various headings, which have their own line heights, but it doesn't seem to when I give it a quick try using my browser's dev tools.

Do you think that would work without having major side effects?

@anechol
Copy link
Author

anechol commented Aug 27, 2024

Thanks for looking into this!

I noticed this doesn't address the issue with text following block quotes, but when I tried to reproduce it using the steps outlined on Jira, the text was wrapped in paragraph tags 🎉 So that's probably not a huge issue going forward, although it will probably still be present in older works.

That being said, there's at least one other place this issue can occur: in dt tags. (I can update the issue to mention this and remove the block quote information.)

I'm wondering if, rather than trying to fix all the individual problem areas, it would make sense to apply the fix to .userstuff itself. I was a little leery that it might affect the various headings, which have their own line heights, but it doesn't seem to when I give it a quick try using my browser's dev tools.

Do you think that would work without having major side effects?

@sarken Thanks for clarifying all this. I think the easiest, albeit "riskier" solution is to just add the line-height to the userstuff class as you mentioned, and yeah, as far as I can tell in the browser, it doesn't seem to be affecting any of the headings. I've gone ahead and committed a change reflecting that. Let me know what you think or if there are any other tags to be mindful of.

@anechol anechol requested a review from sarken August 27, 2024 01:56
Copy link
Member

@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.

This looks good to me now, both in terms of code and in terms of what I see in the browser -- thanks again! (And apologies for not getting back to you sooner; I was under the weather for a few weeks.)

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