-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fix attrs pos #60093
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 attrs pos #60093
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
r? @Manishearth |
ping |
I don't understand what this is fixing, and I don't understand how. When filing such issues please include screenshots, and include a before/after in the PR. Also you've added some parameters to some functions here but it's totally unclear what they do, please document them. |
I thought the display on the issue was clear enough, my bad... The point here is to fix the alignment of the |
There's no attribute there. I see padding, but no attribute. A link to a broken page isn't good enough for this, because that page may not render the same for everyone, and once the bug is fixed it will be hard to know what was broken. Issues should be self-contained and make sense even after being fixed. So should pull requests, the justification for this should be in the comments. The screenshot in a PR helps highlight what was changed by focusing on that. It's less necessary since usually it's just normal, but the screenshot in the issue is pretty important. |
The issue has a broken link, not a screenshot?! Damn it... I'll try to change it later on. Sorry about the mess... |
@Manishearth I added a screenshot on both the issue and the PR. |
@@ -3783,7 +3789,7 @@ const ATTRIBUTE_WHITELIST: &'static [&'static str] = &[ | |||
"non_exhaustive" | |||
]; | |||
|
|||
fn render_attributes(w: &mut dyn fmt::Write, it: &clean::Item) -> fmt::Result { | |||
fn render_attributes(w: &mut dyn fmt::Write, it: &clean::Item, top: bool) -> fmt::Result { |
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.
Still need comments explaining what this parameter does.
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.
code looks good, please add some comments on the function and on the CSS
Yes sir! |
Updated! |
@Manishearth Is my comment good enough or do you have something else in mind? |
@bors r+ |
📌 Commit 180b859 has been approved by |
Fix attrs pos Fixes #60042. Screenshot: <img width="438" alt="Screenshot 2019-05-12 at 15 02 25" src="https://user-images.githubusercontent.com/3050060/57582606-1455ec00-74c7-11e9-9d4e-5ec4da4de7dd.png"> r? @rust-lang/rustdoc
☀️ Test successful - checks-travis, status-appveyor |
Fixes #60042.
Screenshot:
r? @rust-lang/rustdoc