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

Adds option to control code sample overflow styles #68

Closed

Conversation

zachfedor
Copy link
Contributor

What does this PR cover?

By default, code samples are styled so that any overflow on the y-axis is hidden but the x-axis always shows the scrollbar even if it isn't necessary. This can be seen in the first screenshot pasted below, and in my opinion it's not the cleanest of styles.

I changed the default style to be overflow: auto; for both axes. Now, if scrolling is unneeded, it won't show. See the second screenshot.

I also added an option to data.json to allow users to customize this behavior.

How can this be tested?

Serve the master branch and inspect any code block. Serve my branch to see the difference. You can customize my branch's behavior by changing the theme.code_overflow option in data.json to any valid value for the overflow CSS property. An invalid value will fall back to the defaults as defined in the app's CSS file.

Screenshots / Screencast

before:
screen shot 2016-09-05 at 12 38 58 pm
after:
screen shot 2016-09-05 at 12 39 47 pm


Reviewers

Review 1

By adding a +1 you are confirming you have...

  • Witnessed the work behaving as expected (this could be on the author's machine or screencast).
  • Checked for coding anti-patterns.

  • modifies code block styles to overflow: auto; by default
  • adds default code_overflow to theme object in data.json
  • adds code_overflow prop to style overrides in index.html

- modifies code block styles to `overflow: auto;` by default
- adds default `code_overflow` to theme object in `data.json`
- adds `code_overflow` prop to style overrides in `index.html`
@Oxicode
Copy link

Oxicode commented Dec 13, 2016

+1

@RyanHavoc RyanHavoc self-assigned this Dec 13, 2016
@RyanHavoc
Copy link
Member

@zachfedor Apologies for the length of time it has taken for me to review this! What browsers are you seeing the scroll bar appearing when it's not needed?

@zachfedor
Copy link
Contributor Author

@RyanHavoc I'm seeing it pretty much everywhere. I tested on Chrome and Firefox in OSX, Linux, and Windows (also in IE). When I first submitted the issue, I remember you saying you develop on Chrome on OSX. I'm willing to bet the only reason you're not seeing them is a default option in System Preferences. Go into the General group and look for "Show scroll bars." If you set it to "Always," which is how most other operating systems work, you can see the the scrollbars. OSX tries to be uber clean and does some odd hiding-until-needed thing, but they're still there taking up space. I've been burned before by this during some other web projects so I have my prefs set to Always, just in case.

@RyanHavoc RyanHavoc closed this in 271532e Dec 13, 2016
@RyanHavoc
Copy link
Member

@zachfedor I've changed the style directly. I think having the facility to change it in the data.json file is overkill in this instance.

@zachfedor
Copy link
Contributor Author

zachfedor commented Dec 13, 2016

Thanks for reviewing this, @RyanHavoc !

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.

3 participants