Improved code block sizing & color scheme

UX
,
22

I very much agree with this sentiment.

We increased the default font size from 15px to 16px. Changing the code block font size down to 13px is a big shift. Is a couple more characters per line a good trade for readability?

Will inline code styling be updated? At the moment those have different background color, font color, and font family. This is very apparent in posts that interweave a lot of regular text, inline code elements, and code blocks. It makes cross-referencing inline code and block snippets slightly more difficult.

Unrelated to the changes (but related to code blocks) - any ideas for improving the on-hover button icons? At 12px and .7 opacity they’re barely visible (especially when overlapping with code)

image
image452×122 4.9 KB

4 Likes
23

Nice observstion. This was changed due to the buttons being too visible. They were changed to btn-flat but I do see how that may have been too drastic. Maybe this calls for a custom style applied to codeblocks?

1 Like
24

Should we bump the font size up? I was experimenting with 14px yesterday locally and it’s slightly less jarring compared to the default font.

3 Likes
25 
// this is some commented out code

import Component from "@glimmer/component";
import { action } from "@ember/object";
import didInsert from "@ember/render-modifiers/modifiers/did-insert";
import willDestroy from "@ember/render-modifiers/modifiers/will-destroy";
import { service } from "@ember/service";
import DButton from "discourse/components/d-button";
import bodyClass from "discourse/helpers/body-class";
import concatClass from "discourse/helpers/concat-class";
import ReaderModeOptions from "./reader-mode-options";

export default class ReaderModeToggle extends Component {
  @service readerMode;

  get bodyClassText() {
    if (this.readerMode.isTransitioning) {
      return "reader-mode-transitioning reader-mode";
    } else if (this.readerMode.readerModeActive) {
      return "reader-mode";
    } else {
      return "";
    }
  }

  handleDocumentKeydown(e) {
    if (e.ctrlKey && e.altKey && (e.key === "r" || e.key === "®")) {
      this.readerMode.toggleReaderMode();
    }
  }

  @action
  addEventListener() {
    document.addEventListener("keydown", this.handleDocumentKeydown.bind(this));
  }

  @action
  cleanUpEventListener() {
    document.removeEventListener("keydown", this.handleDocumentKeydown);
  }

  <template>
    {{bodyClass this.bodyClassText}}
    <DButton
      {{didInsert this.addEventListener}}
      {{didInsert this.readerMode.setupWidth}}
      {{willDestroy this.cleanUpEventListener}}
      @action={{this.readerMode.toggleReaderMode}}
      @icon="book-reader"
      @preventFocus={{true}}
      title="Toggle Reader Mode (ctrl + alt + r)"
      class={{concatClass
        "icon"
        "btn-default"
        "reader-mode-toggle"
        (if this.readerMode.readerModeActive "active")
      }}
    />
    {{#if this.readerMode.readerModeActive}}
      <ReaderModeOptions />
    {{/if}}
  </template>
}

Here is the latest update :point_up_2:

8 Likes
26

I really like this latest iteration, @jordan-vidrine!

2 Likes
27

To me the latest update is feeling a lot better, I like the colors and sizing is not fighting with our 16px font for text.

def hello
   puts "hello world"
end

Only minor one is that the grey background still feels a bit drab to me and I prefer a tiny bit lighter. But overall I am feeling quite good about this.

@CvX what is your current take?

1 Like
28

Maybe we could use a light version of the tertiary color.

image
image793×121 4.56 KB

rgba(var(--tertiary-rgb), 0.058)

2 Likes
29

I did think of using a color from a theme color palette, but we can’t predict what that would be. It might end up terrible wrong :grimacing:

I like the blue though

1 Like
30

Are you on dark mode or light mode?

The gray I decided to go with is lighter than our previous one (I think). Using var(--primary-50)

1 Like
31

@sam / @fitzy how are you feeling about living with the changes over the past week?

32

IMO we should merge this into core, it looks so much better now.

The old color scheme is making my eyes hurt on old sites :slight_smile:

3 Likes
34

I am definitely in favour of where we’re at now compared to what we have in core.

However, we lost the change to the max-height in commit 98b2763. Was this intentional? I see it was commented out and then deleted in a subsequent commit.

If so, I can still live with a local override.

1 Like
35

Yes it was deleted as to remain as close to our current codeblock size as possible.

1 Like
36

Merging to be done here in this PR → UX: Codeblocks experiment merge by jordanvidrine · Pull Request #29870 · discourse/discourse · GitHub

1 Like
37

Ok, no worries.

1 Like
38

It looks like the new padding is only applied to .hljs elements, which means that simple code blocks (with no highlighting) don’t get it:

hello

console.log("test")

This also causes post heights to jump on initial load, because the highlighting (and therefore the .hljs class) is applied asynchronously.

Can we fix it up so that the padding change applies to codeblocks, even without the .hljs class?

6 Likes
39

Could the darker background also be applied, for consistency?

5 Likes
40

Thanks for noticing these! I have updated those styles to apply to code rather than just hljs blocks.

4 Likes
41

I need to add another fix here. The fix I have added also now incorrectly makes inline code blocks render on their own line.

This is an example of an inline codeblock incorrectly rendering.

2 Likes
42

Link now returns 404. Maybe makes sense to point to the PR?
UX: Codeblocks experiment merge by jordanvidrine · Pull Request #29870 · discourse/discourse · GitHub

Also, the team and I absolutely love this change. It looks so damn good! :heart_eyes:

3 Likes