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

Prevent code from overflowing pre block #34

Merged
merged 12 commits into from
Jan 8, 2016
Merged

Prevent code from overflowing pre block #34

merged 12 commits into from
Jan 8, 2016

Conversation

ozan
Copy link
Contributor

@ozan ozan commented Dec 24, 2015

Quick review anybody?

Fixes #29

Code is prevented form overflowing out of pre block, by a combination of using the more compact of the two monospace fonts more consistently; adding linebreaks; and, overflow: scroll.

@ozan ozan deployed to algos-book-pr-34 January 3, 2016 05:52 Active
@ozan ozan deployed to algos-book-pr-34 January 3, 2016 06:08 Active
@ozan ozan deployed to algos-book-pr-34 January 3, 2016 06:11 Active
@ozan ozan deployed to algos-book-pr-34 January 3, 2016 06:17 Active
@ozan ozan deployed to algos-book-pr-34 January 3, 2016 06:19 Active
@ozan ozan deployed to algos-book-pr-34 January 3, 2016 06:25 Active
@ozan ozan deployed to algos-book-pr-34 January 4, 2016 20:32 Active
@ozan ozan changed the title Prevent code for overflowing pre block Prevent code from overflowing pre block Jan 4, 2016
@@ -31,7 +31,10 @@ class Vertex(object):
self.neighbors[neighbor] = weight

def __str__(self):
return '{} neighbors: {}'.format(self.key, [x.key for x in self.neighbors])
return '{} neighbors: {}'.format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this change actually affect spillover in those boxes? Or does this address character limits in a text editor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This must have been spilling over... the character limits I'm only enforcing in .py files as that's where my linter runs 😄

@smart-alek
Copy link
Contributor

A few things here:

  1. Looks like overflow-x: scroll created a second line at the bottom of each pre block (see below). When I turned off this rule in the Chrome console, I still wasn't seeing any spillover, so maybe it can be safely removed?
  2. Double check me on this, but the other monospaced font seems a tad large for surrounding text when not in a pre block. Whenever a method name is mentioned in prose, it really looks like it's demanding attention. Can we distinguish between that font for its separate use cases?
  3. There were a few changes you had to make (like output_template) that are purely cosmetic. While we want things to look good, would it be easier to increase the width of these pre blocks?

DOUBLE LINES
screen shot 2016-01-05 at 9 00 45 pm

BIG MONOSPACED FONT
screen shot 2016-01-05 at 9 01 23 pm

font-family: Inconsolata, Consolas, Monaco, 'Andale Mono', 'Ubuntu Mono', monospace;
}

pre {
background: rgba(242, 242, 242, 0.5);
padding: 20px;
overflow-x: scroll;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioned it in my general comment, but I think this rule created an extra line at the bottom of each pre box! Catastrophe!

@ozan
Copy link
Contributor Author

ozan commented Jan 7, 2016

Hmm, regarding the big monospaced font, this is what I see:

screen shot 2016-01-06 at 5 09 53 pm

That's both locally and on https://algos-book-pr-34.herokuapp.com/analysis/introduction/

Is the Inconsolata font loading for you? And you're definitely on this branch and did a hard refresh?

@ozan
Copy link
Contributor Author

ozan commented Jan 7, 2016

Re the "extra line" that looks like an OSX scrollbar... I should have overflow: auto rather than overflow: scroll, but I didn't notice since I have OSX configured to only show scrollbars when scrolling. Will fix.

@ozan ozan deployed to algos-book-pr-34 January 7, 2016 01:16 Active
@ozan
Copy link
Contributor Author

ozan commented Jan 7, 2016

Double line issue should be fixed in 97be8f9. Can you take another quick look, and also check that Inconsolata is loading on your set up? Should be good to merge after that.

@ozan
Copy link
Contributor Author

ozan commented Jan 7, 2016

Also re:

While we want things to look good, would it be easier to increase the width of these pre blocks?

I think it's also better code 😄 . In general sticking to the 80-ish char guideline in Python tends to be a good idea.

@ozan ozan deployed to algos-book-pr-34 January 7, 2016 01:27 Active
@smart-alek
Copy link
Contributor

Double line issue seems to be fixed, but Inconsolata still doesn't seem to be loading. Git bisect pointed me to this commit. How else are we loading the Inconsolata font if not in that spot? Previous versions on this branch that look like your screenshots are defaulting to just monospace on my set up. 🔍

@ozan
Copy link
Contributor Author

ozan commented Jan 8, 2016

@aleksharma12 thanks! I've created #38 to track the font issue directly; will merge this in for now.

ozan added a commit that referenced this pull request Jan 8, 2016
Prevent code from overflowing pre block
@ozan ozan merged commit 2b8069e into master Jan 8, 2016
@ozan ozan deleted the 29-spillage branch January 8, 2016 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants