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

Refactor docs generator HTML scaffold #5816

Merged

Conversation

straight-shoota
Copy link
Member

This PR restructures some of the HTML templates for generating API docs. This is a piece extracted from #4755.

  • prefixes partial templates with an underscore to distinguish them from full-page templates
  • extracts repeating content of <head> tag to _head.html partial
  • extracts sidebar to _sidebar.html partial
  • refactors CSS to use flexbox instead of absolute positioning for the page layout

These are all just structural changes without changing any visuals. The only exception is that the sidebar now is always 30 em wide instead of 20% of screen size (which could get pretty tight on small screens).

This dries up sidebar code.
Element IDs for CSS selectors are replaced with classes to reduce specificity in
stylesheets (avoid specificity wars).
Flexbox is better than absolute positioning as it allows for more
flexible layout and dimensions.
@sdogruyol sdogruyol merged commit 90c3a7a into crystal-lang:master Apr 13, 2018
@sdogruyol sdogruyol added this to the Next milestone Apr 13, 2018
@straight-shoota straight-shoota deleted the jm/features/docs-html-structure branch April 13, 2018 08:50
@Sija
Copy link
Contributor

Sija commented Apr 13, 2018

Sidebar got bit (too) wide after this change. Also, now it "jumps" after page change (Chrome 65 on OS X) - to reproduce go to https://crystal-lang.org/api/master/, type search for some sth and hit enter, then on site refresh you'll see that sidebar goes expands in width and goes back to previous state after full site load.

@straight-shoota
Copy link
Member Author

@Sija Hm, yeah that's because the width of the sidebar is influenced by the width of the main body and it adjust after that one is loaded/rendered. You can simulate this by toggling display: none on .main-body in the developer tools.

Now that this PR is merged, I plan to continue with a layout overhaul to make it adaptive for smaller screen sizes. This will be fixed then. I don't think it is worth a hot-fix.

left: 0;
width: 20%;
.sidebar {
width: 30em;
Copy link
Contributor

@RX14 RX14 Apr 13, 2018

Choose a reason for hiding this comment

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

This is unacceptable on vertical monitors. I think it's worth a hot fix because it's simple to use the old 20% value.

Copy link
Member Author

Choose a reason for hiding this comment

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

What exactly is unacceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

That it takes up more than a third of my monitor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Which browser and resolution?

Copy link
Contributor

Choose a reason for hiding this comment

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

firefox, 1200x1920, custom base font size of 20px instead of 16px because idiots designers with their hidpi macbooks chronically make web fonts too small.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, on my MBA screen (1440×900) looks unnaturally wide too... :(

@bcardiff
Copy link
Member

Do you think is a good idea to leave a downloadable version of the docs if the commit message matches /\[doc(s?)\]/? So from ci build anyone would be able to preview things a bit easier.

@RX14
Copy link
Contributor

RX14 commented Apr 13, 2018

@bcardiff why not every PR? We builds the docs in all CI runs anyway (iirc)

@bcardiff
Copy link
Member

@RX14 I didn't want to store unnecessary artifacts. It seems that the quota is per file size and not per repo in circle...

https://circleci.com/docs/2.0/artifacts/

Artifacts are stored on Amazon S3. There is a 3GB curl file size limit.

@straight-shoota
Copy link
Member Author

I don't think having it downloadable is worth it. You can easily checkout the branch and run make docs. A real benefit would be having a preview to browse online.

@Sija
Copy link
Contributor

Sija commented May 23, 2018

@straight-shoota 🏓

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.

6 participants