Change GitHub code block to highlight tag to avoid it overlaps parent div #4121

Merged
merged 1 commit into from Dec 2, 2015

Conversation

Projects
None yet
7 participants
@rebornix
Member

rebornix commented Nov 9, 2015

image

Some code blocks are still using GitHub's code block syntax ```, which lacks proper x-overflow css. Here I change it to highlight tag.

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 10, 2015

Member

Actually, single ` may run into the same situation in small window, like below

image

So I update css for li>pre and p>code, make its max-width as 100% and overflow as auto.

image

BTW, personally I suggest to use highlight block anytime we try to add a block of codes, instead of using GitHub multiline code block syntax.

Member

rebornix commented Nov 10, 2015

Actually, single ` may run into the same situation in small window, like below

image

So I update css for li>pre and p>code, make its max-width as 100% and overflow as auto.

image

BTW, personally I suggest to use highlight block anytime we try to add a block of codes, instead of using GitHub multiline code block syntax.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Nov 10, 2015

Member

This looks better, but the user still has to scroll sideways. The text should wrap within the column. Can you make that update please?

Member

mattr- commented Nov 10, 2015

This looks better, but the user still has to scroll sideways. The text should wrap within the column. Can you make that update please?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 11, 2015

Contributor

Yeah definitely needs to be split up with \ and a line break.

Contributor

envygeeks commented Nov 11, 2015

Yeah definitely needs to be split up with \ and a line break.

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 11, 2015

Member

@mattr- word wrap is good but the catch is, users would have no idea whether the code block is a single line command any more, like below

image

@envygeeks mentioned using \ to split the long code snippet, it's like a content change but it's hard to tell where to break the line, users' window size vastly changes.

Actually this problem only happens when we put single line code block in paragraphs, we can't break the code block like other words. If you guys think a soft word-break like the image shown above is Okay, I'll give it a quick fix 🔨

Member

rebornix commented Nov 11, 2015

@mattr- word wrap is good but the catch is, users would have no idea whether the code block is a single line command any more, like below

image

@envygeeks mentioned using \ to split the long code snippet, it's like a content change but it's hard to tell where to break the line, users' window size vastly changes.

Actually this problem only happens when we put single line code block in paragraphs, we can't break the code block like other words. If you guys think a soft word-break like the image shown above is Okay, I'll give it a quick fix 🔨

@chrisfinazzo

This comment has been minimized.

Show comment
Hide comment
@chrisfinazzo

chrisfinazzo Nov 11, 2015

Contributor

@mattr- Is there a reason adding the highlight class wouldn't work? Interested to know why scrolling sideways is non-optimal in this case. If the plugin install is meant to be a single command (as the docs seem to imply), then a scrolling view makes sense.

@rebornix, Can you try it with the highlight set to "bash" instead of "text"?

Contributor

chrisfinazzo commented Nov 11, 2015

@mattr- Is there a reason adding the highlight class wouldn't work? Interested to know why scrolling sideways is non-optimal in this case. If the plugin install is meant to be a single command (as the docs seem to imply), then a scrolling view makes sense.

@rebornix, Can you try it with the highlight set to "bash" instead of "text"?

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 12, 2015

Member

@chrisfinazzo The reason I use text is that this line of code is likely config instead of a shell command, but bash can give users a better highlighting, I'm Okay with the change. Before I update the pr, let's wait for others' input.

Member

rebornix commented Nov 12, 2015

@chrisfinazzo The reason I use text is that this line of code is likely config instead of a shell command, but bash can give users a better highlighting, I'm Okay with the change. Before I update the pr, let's wait for others' input.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 12, 2015

Contributor

Personally I'm fine either way the choice goes because changing it shouldn't affect anything and if it does then we either have a UI problem on the site or a processing problem in Jekyll with our upstream providers. The shell syntax provides no benefit because we aren't using any builtins for sh/bash.

On the "not knowing the screen sizes" (paraphrased) part, I assume that because we have a fixed layout that adjusts the font size as it gets smaller, in theory if whoever designed the site did it right, there should be no scrolling once the screens get smaller because the responsive side should shrink the font to a size that allows that to happen. I've never tested it though.

Contributor

envygeeks commented Nov 12, 2015

Personally I'm fine either way the choice goes because changing it shouldn't affect anything and if it does then we either have a UI problem on the site or a processing problem in Jekyll with our upstream providers. The shell syntax provides no benefit because we aren't using any builtins for sh/bash.

On the "not knowing the screen sizes" (paraphrased) part, I assume that because we have a fixed layout that adjusts the font size as it gets smaller, in theory if whoever designed the site did it right, there should be no scrolling once the screens get smaller because the responsive side should shrink the font to a size that allows that to happen. I've never tested it though.

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 12, 2015

Member

I agree, the font-size or layout width should be taken care of carefully in mobile view.

The root cause is we are heavily using single-line code block in paragraphs. When we resize our window to 414*736 ( iPhone 6 plus), 6 out of 30 documentation pages are broken, like in collection

image

It just breaks slightly and we can't say <dest>/my_collection/some_subdir/some_doc.html is too long, right?

For now, overflow:auto is an acceptable solution for me or @chrisfinazzo . If Jekyll team has a full plan for responsive font-size shrinking, it can't be better.

Member

rebornix commented Nov 12, 2015

I agree, the font-size or layout width should be taken care of carefully in mobile view.

The root cause is we are heavily using single-line code block in paragraphs. When we resize our window to 414*736 ( iPhone 6 plus), 6 out of 30 documentation pages are broken, like in collection

image

It just breaks slightly and we can't say <dest>/my_collection/some_subdir/some_doc.html is too long, right?

For now, overflow:auto is an acceptable solution for me or @chrisfinazzo . If Jekyll team has a full plan for responsive font-size shrinking, it can't be better.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 12, 2015

Contributor

I'll leave it to @mattr-, @alfredxing and @parkr to decide but I'm gonna be honest, I wouldn't mind if we told CSS it's okay to break in the middle of the word and fold the text to a new line on inline code since it doesn't break anything like paste. I don't know if users will notice it's a single word though.

Contributor

envygeeks commented Nov 12, 2015

I'll leave it to @mattr-, @alfredxing and @parkr to decide but I'm gonna be honest, I wouldn't mind if we told CSS it's okay to break in the middle of the word and fold the text to a new line on inline code since it doesn't break anything like paste. I don't know if users will notice it's a single word though.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Nov 30, 2015

Member

My vote would be to limit the width of the code block, then make the text inside scroll (using max-width: 100%; overflow-x: auto).

Example: http://jsfiddle.net/sn3Lu2zr/

Member

alfredxing commented Nov 30, 2015

My vote would be to limit the width of the code block, then make the text inside scroll (using max-width: 100%; overflow-x: auto).

Example: http://jsfiddle.net/sn3Lu2zr/

@chrisfinazzo

This comment has been minimized.

Show comment
Hide comment
@chrisfinazzo

chrisfinazzo Nov 30, 2015

Contributor

👍🏻 on @alfredxing's change.

Contributor

chrisfinazzo commented Nov 30, 2015

👍🏻 on @alfredxing's change.

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 30, 2015

Member

Rebasing the pr and restrict overflow to x-axis as @alfredxing suggested.

Member

rebornix commented Nov 30, 2015

Rebasing the pr and restrict overflow to x-axis as @alfredxing suggested.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 30, 2015

Contributor

That should be sh, shell or bash, not text.

Contributor

envygeeks commented Nov 30, 2015

That should be sh, shell or bash, not text.

parkr added a commit that referenced this pull request Dec 2, 2015

@parkr parkr merged commit a4d918d into jekyll:master Dec 2, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Dec 2, 2015

@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.