Skip to content

Add footer #1367

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 28, 2021
Merged

Add footer #1367

merged 3 commits into from
Apr 28, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Apr 18, 2021 •
edited
Loading

Fixes #75

Screenshot from 2021-04-21 14-55-57
Screenshot from 2021-04-21 15-28-09

As you can see, whatever the size of the page, it's always at the bottom and not in the middle of the screen but if the content if bigger that the height, you have to scroll to the bottom of the page to be able to see the footer.

Little sidenode for the rustdoc pages in mobile mode. In such case, the footer takes the full width and goes under the sidebar in case it's displayed:

Screenshot from 2021-04-21 15-27-56
Screenshot from 2021-04-21 15-28-00

@jyn514
Copy link
Member

jyn514 commented Apr 18, 2021

I wonder if it makes sense to move the 'About' and homepage links in the bottom. 'About' because it seems common to put that in the footer, and the homepage because to be frank there's not much on our homepage other than search, which we already show in the top-right. That would also free up some space in the header.

jyn514
jyn514 reviewed Apr 18, 2021
View reviewed changes
@Nemo157
Copy link
Member

Nemo157 commented Apr 19, 2021

Is there a reason to not make the footer fullwidth on rustdoc pages?

The other page it'd be useful to verify it works on is rustdoc-src pages, since they don't have the sidebar have a collapsible sidebar.

@GuillaumeGomez
Copy link
Member Author

@Nemo157 You'll see in a bit. ;)

@GuillaumeGomez GuillaumeGomez marked this pull request as ready for review April 19, 2021 15:41
@GuillaumeGomez
Copy link
Member Author

Updated the images and the PR. With this, I think it's now ready for review.

@Nemo157
Copy link
Member

Nemo157 commented Apr 19, 2021

Ah, so the footer scrolls into view on long rustdoc pages, so that's why it can't be across both the page and sidebar. Still seems a bit weird in that it implies that it's part of the rustdoc output; maybe having some kind of DOCS.RS wordmark in the footer would help avoid that.

On rustdoc-src pages it doesn't extend all the way left:

image

Nemo157
Nemo157 reviewed Apr 19, 2021
View reviewed changes
@GuillaumeGomez
Copy link
Member Author

Fixed the position on the source code page and updated "Privacy" to "Privacy policy".

@jyn514 jyn514 added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Apr 19, 2021
pietroalbini
pietroalbini reviewed Apr 19, 2021
View reviewed changes
@pietroalbini
Copy link
Member

Another thing I noticed is that the footer on rustdoc pages uses the rustdoc serif font, could it be switched to use docs.rs's main font?

@GuillaumeGomez
Copy link
Member Author

@pietroalbini Wow, you have good eye. Didn't spot this one at all!

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Apr 20, 2021 •
edited
Loading

Updated the links, added icons, removed links from the top navbar, changed the minimum width for the resize handling and updated the screenshots.

jyn514
jyn514 reviewed Apr 20, 2021
View reviewed changes
@pietroalbini
Copy link
Member

Hmm, looking at the screenshots the icons feels too prominent? I think I prefer the icon-less version.

@GuillaumeGomez
Copy link
Member Author

So before removing them again, what do you think @rust-lang/docs-rs ? :)

@GuillaumeGomez
Copy link
Member Author

Removed the icons and updated the screenshots.

@pietroalbini
Copy link
Member

The font in rustdoc pages still seems to be serif. Otherwise that looks great!

@GuillaumeGomez GuillaumeGomez force-pushed the footer branch 2 times, most recently from 150dbbe to cae575e Compare April 21, 2021 13:31
@GuillaumeGomez
Copy link
Member Author

@pietroalbini You're absolutely right! I updated the font so it's now sans-serif. :)

@Nemo157
Copy link
Member

Nemo157 commented Apr 21, 2021

Found one fun bug, the footer renders at the bottom of the initial page load and scrolls with the content on non-rustdoc pages.

image

@GuillaumeGomez
Copy link
Member Author

Nice!

@GuillaumeGomez
Copy link
Member Author

So there were actually two issues:

  1. CSS position is never relative by default (uuuurg) so I had to set it as such on <body>.
  2. Some docs.rs pages were using margin-bottom or didn't have bottom margin at all, so I used padding-bottom in both cases, so on pages that didn't use bottom margin, the end of the page won't be right under the text. It's actually a nice improvement, I never paid attention to that before. :)

@Nemo157
Copy link
Member

Nemo157 commented Apr 27, 2021

Looks like the about pages could do with a little extra padding-bottom too:

image

@GuillaumeGomez
Copy link
Member Author

I don't have the issue on the /builds page:

Screenshot from 2021-04-27 10-50-53

@Nemo157
Copy link
Member

Nemo157 commented Apr 27, 2021

/<crate>/builds is different from /about/builds

@GuillaumeGomez
Copy link
Member Author

Indeed, also I confirmed that this issue only appeared on mobile.

@GuillaumeGomez
Copy link
Member Author

Fixed! So I applied @Nemo157's suggestion and added bottom padding on the about pages (which forced me to update the DOM and create a new CSS class to correctly handle it).

Nemo157
Nemo157 approved these changes Apr 27, 2021
View reviewed changes
Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

Styling all looks good to me now, I don't have much of an opinion on what links are in it.

syphar
syphar approved these changes Apr 27, 2021
View reviewed changes
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

I did a round of tests, in Firefox/Chrome and in mobile sizes.

All seems fine to me

@GuillaumeGomez GuillaumeGomez merged commit 2318535 into rust-lang:master Apr 28, 2021
@GuillaumeGomez GuillaumeGomez deleted the footer branch April 28, 2021 21:16
@GuillaumeGomez GuillaumeGomez mentioned this pull request Apr 29, 2021
Merged
@syphar syphar mentioned this pull request Jul 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietroalbini pietroalbini pietroalbini left review comments

@jyn514 jyn514 jyn514 left review comments

@Nemo157 Nemo157 Nemo157 approved these changes

@syphar syphar syphar approved these changes

Assignees
No one assigned
Labels
S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Advertising and tracking policy
5 participants
@GuillaumeGomez @jyn514 @Nemo157 @pietroalbini @syphar