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

Wrap lines at 80 characters #3616

Closed
wants to merge 1 commit into from
Closed

Wrap lines at 80 characters #3616

wants to merge 1 commit into from

Conversation

chrisfinazzo
Copy link
Contributor

No description provided.

@chrisfinazzo chrisfinazzo mentioned this pull request Mar 24, 2015
@@ -52,7 +53,8 @@ your <code>_config.yml</code> file, with the addition of the preceding <code>_</
</p>
</div>

### Step 3: Optionally render your collection's documents into independent files
### Step 3: Optionally render your collection's documents into independent
files
Copy link
Member

Choose a reason for hiding this comment

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

this breaks the header i think

@parkr
Copy link
Member

parkr commented Mar 24, 2015

some weird indentation stuff and some formatting things go wrong. looks like we can't just broad-stroke it, have to be careful about what we change

@@ -32,7 +32,8 @@ class="flag">flags</code> (specified on the command-line) that control them.
<tr class="setting">
<td>
<p class="name"><strong>Site Source</strong></p>
<p class="description">Change the directory where Jekyll will read files</p>
<p class="description">Change the directory where Jekyll will
read files</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this should improve the readability?
Personally I would like to see it much more like this:

<p class="description">Change the directory where Jekyll will read files</p>

or if the line has to be shorter, like this:

<p class="description">
    Change the directory where Jekyll will read files
</p>

Same comment for a few other occurrences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I need to look more closely at the tables, code blocks, and other things inside of <pre> tags. My original intent was just to get everything under 80 characters for each line, but I can see there is more work that needs to be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Paragraphs are a block tag, there must always be a new line after the <p> just so you know, those are just the unwritten style rules for HTML. If it's a block level element imply it's block level by giving it a new line, if it's inline then imply it's inline without a new line.

@chrisfinazzo
Copy link
Contributor Author

I know there are at least a few places where it's not possible to move lines around because it's inside of a code block or part of a long URL. A bit surprised that I missed it considering that these are just regular paragraphs and a header, but I'll look into it and try to shape things up.

For the record, inline links are hideous. Did someone on the Core team make that decision or did this just happen organically over time because GitHub encourages long URLs in their Markdown docs?

I would create a PR to "fix" that, but I imagine it would be massive as it touches almost everything inside of site.

@envygeeks
Copy link
Contributor

For what it's worth my vote will always go towards whatever makes the source of the docs more readable, so if you think it makes things more readable I think you should put it in if you like and see what everyone else thinks. It never hurts to improve readability in all places!

@chrisfinazzo chrisfinazzo mentioned this pull request Apr 6, 2015
@parkr
Copy link
Member

parkr commented Apr 10, 2015

I guess in principle we'd want this, but it isn't worth it at this point. Thank you though!! ❤️

@parkr parkr closed this Apr 10, 2015
@chrisfinazzo chrisfinazzo deleted the wrap-lines branch September 23, 2015 14:53
@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants