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

Remove wyrm_addons/pygments #504

Merged
merged 10 commits into from
Jan 14, 2018
Merged

Remove wyrm_addons/pygments #504

merged 10 commits into from
Jan 14, 2018

Conversation

Blendify
Copy link
Member

@Blendify Blendify commented Dec 16, 2017

Fixes #503 By removing wyrm_addons/pygments we get full support of using native pygments to style code blocks

@Blendify
Copy link
Member Author

This is not ready for review because I still need to fix things like text and spacing in our own sass

@Blendify
Copy link
Member Author

This is ready for review now

@jessetan
Copy link
Contributor

Looks pretty good as far as I can see, although the line-height feels a bit too much considering the font size and shape (I'm using macOS, which has Consolas). Previously the line-height was normal (== 1.2), now it is 1.5.
Screenshots below of possible other values. 1.5 looks better to me if the fontsize is a bit larger (14px?), but maybe I'm just nitpicking.

  • line-height: 1.2 (current value in 0.2.4)
    1 2
  • line-height: 1.3
    1 3
  • line-height: 1.4
    1 4
  • line-height: 1.5 (new value)
    1 5

padding: $base-line-height / 2 $base-line-height / 2
font-family: $code-font-family
font-size: 12px
line-height: normal
Copy link
Contributor

@jessetan jessetan Dec 20, 2017

Choose a reason for hiding this comment

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

font-size and line-height can be moved to a new CSS rule for line numbering and actual code. That will prevent them from going out of sync in future.

pre.literal-block, div[class^='highlight'] pre, .linenodiv pre
    font-size: 12px
    line-height: normal

Adding pre.literal-block to this rule also aligns the font size for literal blocks, which look out of place now:
literal code block

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

margin: 0
padding: $base-line-height / 2 $base-line-height / 2
font-family: $code-font-family
font-size: 12px
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps font-size: $base-font-size * 0.75 ?
(Also in other places in this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

This would be a new behavior correct? Seems fine but maybe this is better for another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

The behavior would be the same ($base-font-size is 16px), but indeed perhaps better to move to a new PR that adds a $code-font-size in _theme_variables.sass so we can be sure we have consistent font sizes.

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.

2 participants