Skip to content

Cleanup highlight tag#9177

Merged
jekyllbot merged 5 commits intojekyll:masterfrom
ashmaroli:cleanup-highlight-tag
Nov 7, 2022
Merged

Cleanup highlight tag#9177
jekyllbot merged 5 commits intojekyll:masterfrom
ashmaroli:cleanup-highlight-tag

Conversation

@ashmaroli
Copy link
Copy Markdown
Member

  • This is a 🔨 code refactoring proposal.
  • Existing tests cover changed lines.

Summary

Some internal changes to improve Jekyll::Tags::HighlightBlock

@ashmaroli ashmaroli requested a review from parkr November 6, 2022 17:48
"class=\"language-#{@lang.to_s.tr("+", "-")}\"",
"data-lang=\"#{@lang}\"",
%(class="language-#{@lang.to_s.tr("+", "-")}"),
%(data-lang="#{@lang}"),
Copy link
Copy Markdown
Contributor

@yboulkaid yboulkaid Nov 6, 2022

Choose a reason for hiding this comment

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

One small (non-blocking) suggestion: we could put everything in one line to avoid having to go through an array and join its contents:

code_attributes = %(class="language-#{@lang.to_s.tr("+", "-")}" data-lang="#{@lang}")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Theoretically, we could have this in one line. But I think that would increase the line-length greater than 100 chars (as enforced by our rubocop config. Regardless, I cannot test this myself at the moment).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just tested it and it fits under the 100 char length limit :)

Copy link
Copy Markdown
Contributor

@yboulkaid yboulkaid left a comment

Choose a reason for hiding this comment

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

All cosmetic changes that make the code better 👍

@ashmaroli
Copy link
Copy Markdown
Member Author

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit f40e5cf into jekyll:master Nov 7, 2022
jekyllbot added a commit that referenced this pull request Nov 7, 2022
@ashmaroli ashmaroli deleted the cleanup-highlight-tag branch November 7, 2022 06:54
github-actions bot pushed a commit that referenced this pull request Nov 7, 2022
Ashwin Maroli: Cleanup highlight tag (#9177)

Merge pull request 9177
Copy link
Copy Markdown

@khaledalam173 khaledalam173 left a comment

Choose a reason for hiding this comment

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

New

@jekyll jekyll locked and limited conversation to collaborators Nov 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants