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-6622 Decrease margins on definition lists in FAQs #4756

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

Conversation

forceofcalm
Copy link
Contributor

Pull Request Checklist

Issue

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

Purpose

Adjusts the margins for .userstuff dl and dd elements within .faq containers to be smaller.

Testing Instructions

These margins can be seen on the /faq path (although there does not seem to be any FAQ data on my local...)

Credit

calm (they/them)

@@ -1,10 +1,11 @@
/* ==USERSTUFF: displaying content inputted by a user into a textarea.*/

.userstuff {
word-wrap: break-word;
word-wrap: break-word;
Copy link
Member

Choose a reason for hiding this comment

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

Could you put the spacing back here? Our house style (developed many, many years ago) is to list CSS3 properties last and indent them four spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah. That's my linter. Let me turn that off for this.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like this line got fixed! However, all of the other properties are now indented 2 spaces instead of 4. Can you please restore the original indenting across the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All lines should have been fixed with that commit. I just checked again on the diff and it all looks good?
Screenshot from this morning, from my IDE too.
This is a screenshot from my IDE this morning.
Not sure if GH is just being slow to catch up, or what.

Copy link
Member

Choose a reason for hiding this comment

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

The screenshot matches what GitHub is showing, but the properties should be indented just two spaces, putting them where the second vertical line is. Basically, the lines were correctly indented in the original version of the file and should be unchanged in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, so then I'm confused about the original comment, asking to indent them all 4 spaces?

Or does that mean just for CSS3 properties (indent 4), and previous versions are all indented 2?

Copy link
Member

Choose a reason for hiding this comment

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

Right, just the CSS3 properties get indented 4 spaces (and go last), and the rest get indented 2. Sorry for the confusion!

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 this! Docs is really going to appreciate it. (Also, apologies for the premature single comment. I was distracted and hit the wrong button.)

If you want to add FAQ data to your dev environment, you can create an admin account with the superadmin, translation, or docs role to access the interface for creating FAQs.

public/stylesheets/site/2.0/21-userstuff.css Outdated Show resolved Hide resolved
public/stylesheets/site/2.0/21-userstuff.css Outdated Show resolved Hide resolved
@forceofcalm forceofcalm requested a review from sarken March 14, 2024 20:38
width: auto;
opacity: 1;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
opacity: 1;
opacity: 1;

(If you compare it to word-wrap on line 4, you can see opacity is currently indented a total of 6 spaces instead of the desired 4.)

Comment on lines +343 to +348
it "is valid even if the email casing is different" do
legit_user.email = legit_user.email.upcase
User.current_user = legit_user
expect(safe_report.save).to be_truthy
end

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it ended up on the wrong branch, did you mean to to add this to #4755? (If yes, it worries me a bit that the test didn't fail on this branch. It suggests that it's not testing what it's meant to.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants