Apply code highlighting background to code in HTML headers `<h1>` through `<h6>` #2771

ElectricRCAircraftGuy · 2020-12-31T08:58:55Z

This is an enhancement or feature.

Summary

Modify _base.scss to apply code highlighting background to code in headers. It now looks like this. Notice the code in headers now has a grey background. Before this PR, code in headers would NOT get the grey background, which made it very hard to tell it was code!
.
Darken background color from #fafafa to $lighter-gray, since the former is so light it's hard to see.
Add Table of Contents to README. See this commit for details: cd2726c.
Update Rakefile to serve jekyll on port 4001 instead of 4000, so as to avoid conflicts with NoMachine. See my answer here for more background and info on this.
Update changelog.

Context

...instead of port 4000, since port 4000 interferes with programs such as [NoMachine](https://www.nomachine.com/).

...using the Sublime Text editor [MarkdownTOC](https://github.com/naokazuterada/MarkdownTOC) tool! Here are my user settings for it: ```json { "defaults": { "autoanchor": true, "autolink": true, "bracket": "round", "levels": [1,2,3,4,5,6], "indent": "\t", "remove_image": true, "link_prefix": "", "bullets": ["-"], "lowercase": "only_ascii", "style": "ordered", "uri_encoding": true, "markdown_preview": "" }, "id_replacements": [ { "pattern": "\\s+", "replacement": "-" }, { "pattern": "<|>|&|'|"|&mmistakes#60;|&mmistakes#62;|&mmistakes#38;|&mmistakes#39;|&mmistakes#34;|!|#|$|&|'|\$|\$|\\*|\\+|,|/|:|;|=|\\?|@|\\[|\\]|`|\"|\\.|\\\\|<|>|{|}|™|®|©|%", "replacement": "" } ], "logging": false } ```

ElectricRCAircraftGuy · 2020-12-31T09:05:45Z

The Table of Contents in the readme can be viewed here, just above the screenshots: https://github.com/ElectricRCAircraftGuy/minimal-mistakes/tree/upstream-theme_fixes. I made it collapsed by default so it doesn't distract from the main content.

iBug

This is a bit too messy. Changing large chunks of README that doesn't have much to do with the original intention isn't a good practice, nor do I like it though.

My real question is, is this change really useful to most users? Given that the CSS change is simple enough for one to apply on their own, and that some may argue that the result is not desired, I don't see much value in this PR.

iBug · 2021-01-16T17:38:53Z

README.md

@@ -15,13 +15,42 @@ Minimal Mistakes is a flexible two-column Jekyll theme, perfect for building per

 **Note:** The theme uses the [jekyll-include-cache](https://github.com/benbalter/jekyll-include-cache) plugin which will need to be installed in your `Gemfile` and added to the `plugins` array of `_config.yml`. Otherwise you'll encounter `Unknown tag 'include_cached'` errors at build.

+## Table of Contents


Uh, what's the point of this?

It's a Table of Contents for the readme. It helps orient people quickly to the topics of interest, and easily jump around. Explore this readme live and interactively here: https://github.com/ElectricRCAircraftGuy/minimal-mistakes/tree/upstream-theme_fixes.

It looks like this:

Notice there's an arrow to "click to expand" and view the whole Table of Contents. It's super useful I think. I really like it.

iBug · 2021-01-16T17:39:28Z

Rakefile

@@ -37,7 +37,8 @@ task :preview do
    "destination"   => base.join('test/_site').to_s,
    "force_polling" => false,
    "serving"       => true,
-    "theme"         => "minimal-mistakes-jekyll"
+    "theme"         => "minimal-mistakes-jekyll",
+    "port"          => 4001,


This probably wasn't intended to be committed, right?

No, this is very much intentional. It solves the NoMachine problem I describe above, and causes no harm to anyone not using NoMachine.

iBug · 2021-01-16T17:41:19Z

README.md

 ## Development

 To set up your environment to develop this theme, run `bundle install`.

-To test the theme, run `bundle exec rake preview` and open your browser at `http://localhost:4000/test/`. This starts a Jekyll server using content in the `test/` directory. As modifications are made to the theme and test site, it will regenerate and you should see the changes in the browser after a refresh.
+To test the theme, run `bundle exec rake preview` and open your browser at [`http://localhost:4001/test/`](http://localhost:4001/test/). This starts a Jekyll server using content in the `test/` directory. As modifications are made to the theme and test site, it will regenerate. Note that you will see the changes in your browser only after doing a manual page refresh.


Still don't quite get the point of this change. The wording seems fine, though.

See bullet 4 in my PR description, where I explained it:

Update Rakefile to serve jekyll on port 4001 instead of 4000, so as to avoid conflicts with NoMachine. See my answer here for more background and info on this.

For anyone who uses NoMachine, which is a very popular remote login tool, it uses port 4000 too and conflicts with the web server during website testing. This fixes that. For anyone else using NoMachine, it solves their problem without them having to search around a bunch to figure it out, and for everyone else it causes no harm, so is overall a useful change.

I haven't even heard about NoMachine (I use SSH and VS Code). It's not convincing to change the default port because of a software that has no relation with Jekyll. FYI: I have localhost:4000 saved in my browser bookmark for quick access, and this is going to break it.

ElectricRCAircraftGuy · 2021-01-17T03:20:25Z

This is a bit too messy.

I agree this is 3 small changes in one PR, but that's kind of hurtful. I've put great effort into making this PR clean, neat, easy to read, easy to understand, and well-described in the PR's description.

This PR is 3 small changes in one:

Apply code highlighting background to code in HTML headers <h1> through <h6>
Move the server port from 4000 to 4001 to avoid conflicts with users using NoMachine, such as myself, so I can even build the site and view it. This hurts no-one and helps some.
Add a table of contents to the readme to make it easier to jump around and read.

Changing large chunks of README that doesn't have much to do with the original intention isn't a good practice, nor do I like it though.

If someone with approval and merge authority wants me to split the PR, I can. Or, they can commandeer the PR and do it. If they want to accept only parts of it, the fastest way is for them to manually pull my branch, fix it up how they want it, and then merge it to the repo. My changes they want do get in, and any they don't, don't.

My real question is, is this change really useful to most users? Given that the CSS change is simple enough for one to apply on their own, and that some may argue that the result is not desired, I don't see much value in this PR.

On all 3 of the main markdown-based interactive sites in the world: Stack Overflow, GitHub, and GitLab, headers with code have the code shaded, like this:

I'm a header `with code`

Unfortunately, minimal mistakes theme doesn't follow this pattern. We can help meet a lot of people's expectations by merging this.

How many people are not web developers who use Jekyll, GitHub pages, and this minimal mistakes theme? I'd argue that it's most users. I am not a web developer. I didn't know hardly anything about CSS before I put in the 50+ or so hours tweaking this theme. It's very difficult for an aeronautical engineer (ex: me) or embedded software developer (also me) to learn to do these CSS changes. I find huge value in this PR. I wish someone had done it before me. I think others will be grateful for it too. Anyone who writes anything in markdown ever will notice when code doesn't show up in headings. It's one of the first things I ever noticed about this theme. I write a lot on Stack Overflow and GitHub, and this theme was missing that feature. I considered that a bug.

@iBug , thanks for reviewing. Do you have approval or merge authority on this repo?

I'd like to hear from @mmistakes too.

iBug · 2021-01-17T06:08:44Z

I don't have any extra access to this repo than you do, but I'm sure @mmistakes's thoughts would be in line with mine.

Questions to ask before deciding a feature is useful:

Is there a strong demand for this?
- Code background in headings: Mixed, some may prefer the status quo
- README ToC: Good to have
- Default listening port change: Probably no. Users are responsible for knowing their software. What's more, this repo is not intended to be forked for building your website (the starter repo is).
Does it complicate things or make it harder to maintain code?
- Code background: It's simple enough. Nothing wrong with this.
- README ToC: Apparently it does. Maintaining the ToC (and the unnecessary anchors - headings already have id attribute added automatically by GitHub) now requires extra effort.
- Default listening port change: As said above. it's not a compelling reason to change Jekyll's default behavior just because another (not quite popular) software conflicts with it. It's going to cause confusion for whoever using the default setup.
Does it break for existing users, or is it easy enough for one to apply on their own?
- Code background: Some users may simply don't want this change. Plus this CSS is straightforward to understand and apply.
- README ToC: Not end-user facing, not discussing here.
- Default port: This would greatly break existing setup and is very easy to apply.

Given the above arguments, I'm in favor of none of these changes. Let the power that be decide - I have no authority.

ElectricRCAircraftGuy · 2021-01-17T07:06:14Z

Maintaining the ToC [and anchors]...now requires extra effort.

This isn't quite true. It depends on your text editor. The ToC I added uses MarkdownTOC, which is a Sublime Text plugin. These markers:  work with the tool and every time I save the file the ToC is automatically updated without any intervention from me. It automatically updates all HTML anchors too.

Plus this CSS is straightforward to understand and apply.

I still disagree with this and think it would do a lot of people good. We could help many people by merging this change. If @mmistakes doesn't implement this change/merge this PR, I hope he at least documents what I've done officially. It took me a long time of research, spread out over months in evenings after work, and on weekends, to figure this and other things out. Again, I, and most others doing Jekyll pages I think, have no prior web or HTML or real CSS experience.

iBug · 2021-01-17T07:51:25Z

While maintaining the ToC appears easy for you, it requires one to use Sublime Text and use that specific plugin, which obviously everyone does not. You can say "it's easy according to your workflow", but it doesn't necessarily hold true for someone else. The same holds for your other arguments.

The point here is, you speak from your experiences and perspectives, but as a 7.5k-star open-source project, we have to take other users' favor and dependency into account. That something works for you in no way implies it'll also work for someone else.

Unless there's a compelling reason to, default settings shouldn't be changed, nor should breaking changes be introduced. That's my $0.02.

ElectricRCAircraftGuy · 2021-01-17T08:00:03Z

nor should breaking changes be introduced.

Let's not consider having to update a single optional bookmark in one's browser from http://localhost:4000/test/ to http://localhost:4001/test/ a "breaking change." That's a bit of a stretch.

iBug · 2021-01-17T08:28:29Z

I don't want to be too harsh, but here's my view on all three changes.

The core: CSS changes

The only change related to the title of this PR is code background is a few lines of new CSS, which could be simplified as such:

h1, h2, h3, h4, h5, h6 {
  > code {
    background: $code-background-color;
  }
}

After taking a closer look, I noticed that it isn't carefully done. For example, someone using code-in-heading combined with header overlay sees an ugly artifact:

So at best this CSS should be relocated inside .page__content here. Another good-to-have change would be replacing the CSS key with background-color so it doesn't override other background-related settings.

This is one way the change is breaking, though it can be easily fixed.

Again, just because you want this change doesn't mean someone else does. This is resembled by the fact that all your linked issues have no input or interaction from other users (excluding Michael, of course). See #2246 for example: Someone else opened that issue, and 3 more users showed interest in that feature.

And more, see how the result is only a few lines of CSS with no complicated nesting or location?

before I put in the 50+ or so hours tweaking this theme

That's not a fault of the theme. Someone with basic knowledge of CSS should be able to achieve this in a few minutes. There are plenty of introductory CSS tutorials online, and it doesn't even cost you an hour to learn the required level of knowledge to be able to figure this out.

The Readme ToC

I'll basically reiterate my last commment. You said you generated the ToC "easily" with your setup (Sublime Text + a plugin). In order for the same task to become comparably easy for someone else, they have to at best follow your setup, install and open Sublime Text and the plugin to update the ToC, after their own favorite workflow. For example, I use VS Code as my code editor / IDE, and occasioanlly Typora as my Markdown editor. It's nowhere near compelling that I have to add Sublime Text just for this ToC.

The port change

I'll put this point first: This repository is not intended for end users who want to build a website with this theme. It is therefore assumed that someone running rake preview or similar is a theme developer, not user. Based on this role assignment, it's then the developer's responsibility to know what tools they're using, and resolve software conflict by themselves.

Changing one bookmark for me surely isn't a great deal of a problem, making more people have to change their bookmarks is.

Setting that aside, it's not this theme's fault that the port conflicts with another software, so it makes less sense to "fix it" here than to fix it with Jekyll. Not to mention that NoMachine isn't as popular as other remote desktop (RD) solutions like VNC (default port 5900) and RDP (default port 3389). 4000 is a port just too common for a conflict, so I'd even doubt if a long-running software should use it.

Given that the port can be specified per invocation with command-line options, this is not a desirable change to have.

Summing up

So basically you squashed 3 unrelated changes into one PR that arguably makes reviewing and cherry-picking more complex than it should, and in addition this PR introduced

A change in theme appearance that is not perceived to be popular / demanded
A change in the README document that requires extra effort or software-and-workflow to maintain
A change in software setting that is both confusing and targetting the wrong audience

I'll copy my word from the previous comment as the last line, with emphasis added:

The point here is, you speak from your experiences and perspectives, but as a 7.5k-star open-source project, we have to take other users' favor and dependency into account. That something works for you in no way implies it'll also work for someone else.

iBug · 2021-03-26T13:14:53Z

Now your README change is useless.

https://twitter.com/natfriedman/status/1375430850620256257

github-actions · 2021-05-26T03:13:05Z

This pull request has been automatically marked as stale because it has not had recent activity.

This pull request will automatically be closed in 7 days if no further activity occurs. Thank you for all your contributions.

ElectricRCAircraftGuy · 2021-05-26T07:54:20Z

Bump. @mmistakes

iBug · 2021-05-26T08:26:36Z

I'm 100% sure Michael wouldn't like this PR in its current state. The only change I have hope that Michael may appreciate is the color change from #fafafa to $lighter-gray. Trust me or not, you'd better drop everything else before expecting any positive input from Michael.

mmistakes · 2021-05-26T12:48:00Z

My views are completely in alignment with @iBug's comments.
I prefer focused pull requests that address one feature or bug... adding several no matter how small they appear makes merging harder.

Fixing the code block background seems like a no brainer to me, but it needs to be the only commit for the pull request. It also needs to be tested across all the skins to make sure it looks good in all the different color schemes.

github-actions · 2021-07-27T02:16:27Z

This pull request has been automatically marked as stale because it has not had recent activity.

This pull request will automatically be closed in 7 days if no further activity occurs. Thank you for all your contributions.

ElectricRCAircraftGuy added 4 commits December 31, 2020 00:04

Rakefile & README.md: make rake open jekyll at port 4001...

a218f88

...instead of port 4000, since port 4000 interferes with programs such as [NoMachine](https://www.nomachine.com/).

_base.scss: apply code background color to all 6 HTML heading tags

2c483c2

Update changelog with PR link

72e52b2

Do minor update for clarity in README

899eff8

ElectricRCAircraftGuy mentioned this pull request Jan 8, 2021

Add the expected grey background to code snippets in backticks inside headings #2780

Closed

iBug requested changes Jan 16, 2021

View reviewed changes

iBug referenced this pull request May 13, 2021

Update stale.yml

635b179

github-actions bot added the Status: Stale label May 26, 2021

mmistakes added the Type: Enhancement label May 26, 2021

github-actions bot removed the Status: Stale label May 27, 2021

github-actions bot added the Status: Stale label Jul 27, 2021

github-actions bot closed this Aug 3, 2021

ElectricRCAircraftGuy mentioned this pull request Jun 18, 2023

Fix code syntax highlighting colors ElectricRCAircraftGuy/ElectricRCAircraftGuy.github.io#16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply code highlighting background to code in HTML headers `<h1>` through `<h6>` #2771

Apply code highlighting background to code in HTML headers `<h1>` through `<h6>` #2771

ElectricRCAircraftGuy commented Dec 31, 2020 •

edited

Loading

ElectricRCAircraftGuy commented Dec 31, 2020

iBug left a comment

iBug Jan 16, 2021

ElectricRCAircraftGuy Jan 17, 2021

iBug Jan 16, 2021

ElectricRCAircraftGuy Jan 17, 2021

iBug Jan 16, 2021

ElectricRCAircraftGuy Jan 17, 2021

iBug Jan 17, 2021

ElectricRCAircraftGuy commented Jan 17, 2021

iBug commented Jan 17, 2021

ElectricRCAircraftGuy commented Jan 17, 2021 •

edited

Loading

iBug commented Jan 17, 2021

ElectricRCAircraftGuy commented Jan 17, 2021 •

edited

Loading

iBug commented Jan 17, 2021 •

edited

Loading

iBug commented Mar 26, 2021

github-actions bot commented May 26, 2021

ElectricRCAircraftGuy commented May 26, 2021

iBug commented May 26, 2021

mmistakes commented May 26, 2021

github-actions bot commented Jul 27, 2021

		@@ -15,13 +15,42 @@ Minimal Mistakes is a flexible two-column Jekyll theme, perfect for building per

		Note: The theme uses the [jekyll-include-cache](https://github.com/benbalter/jekyll-include-cache) plugin which will need to be installed in your `Gemfile` and added to the `plugins` array of `_config.yml`. Otherwise you'll encounter `Unknown tag 'include_cached'` errors at build.

		## Table of Contents

Apply code highlighting background to code in HTML headers <h1> through <h6> #2771

Apply code highlighting background to code in HTML headers <h1> through <h6> #2771

Conversation

ElectricRCAircraftGuy commented Dec 31, 2020 • edited Loading

Summary

Context

ElectricRCAircraftGuy commented Dec 31, 2020

iBug left a comment

Choose a reason for hiding this comment

iBug Jan 16, 2021

Choose a reason for hiding this comment

ElectricRCAircraftGuy Jan 17, 2021

Choose a reason for hiding this comment

iBug Jan 16, 2021

Choose a reason for hiding this comment

ElectricRCAircraftGuy Jan 17, 2021

Choose a reason for hiding this comment

iBug Jan 16, 2021

Choose a reason for hiding this comment

ElectricRCAircraftGuy Jan 17, 2021

Choose a reason for hiding this comment

iBug Jan 17, 2021

Choose a reason for hiding this comment

ElectricRCAircraftGuy commented Jan 17, 2021

I'm a header with code

iBug commented Jan 17, 2021

ElectricRCAircraftGuy commented Jan 17, 2021 • edited Loading

iBug commented Jan 17, 2021

ElectricRCAircraftGuy commented Jan 17, 2021 • edited Loading

iBug commented Jan 17, 2021 • edited Loading

The core: CSS changes

The Readme ToC

The port change

Summing up

iBug commented Mar 26, 2021

github-actions bot commented May 26, 2021

ElectricRCAircraftGuy commented May 26, 2021

iBug commented May 26, 2021

mmistakes commented May 26, 2021

github-actions bot commented Jul 27, 2021

Apply code highlighting background to code in HTML headers `<h1>` through `<h6>` #2771

Apply code highlighting background to code in HTML headers `<h1>` through `<h6>` #2771

ElectricRCAircraftGuy commented Dec 31, 2020 •

edited

Loading

I'm a header `with code`

ElectricRCAircraftGuy commented Jan 17, 2021 •

edited

Loading

ElectricRCAircraftGuy commented Jan 17, 2021 •

edited

Loading

iBug commented Jan 17, 2021 •

edited

Loading