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

Fix: highlight active line in language-specified codeblock #289

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

alythobani
Copy link

@alythobani alythobani commented Oct 14, 2024

Description

Fixes #288 as described in the "Additional context / Possible solution" section. I.e., this PR now has the active codeblock line highlighted by updating the --gradient-background-colour variable instead of the background property (which was getting overridden by the update around line 144 in codeblocks.css).

Note also that as mentioned in #288, a fix that would overlay highlights (e.g. active highlight layered on top of hl highlight, so that user can still tell the current line has another highlight on it) could be more ideal than this fix, where the hl highlight is fully replaced by the active-line highlight. But this PR's approach might be ideal as a quick initial fix without needing to adjust the DOM structure (e.g. adding another div to allow for overlapping highlights).

There are also multiple !important flags involved here, adding to the complexity, so a fully ideal fix would maybe involve replacing those with a different approach. But likewise here, this PR's approach could be better as a simpler fix for now.

Motivation and Context

Fixes #288

How has this been tested?

Tested locally by updating styles.css with the described line change, and seeing it work.

This change may or may not affect other areas of code; I haven't spent too long diving into this so far, just came up with this quick potential fix after some reading and experimentation.

Screenshots

Before fix:

Screen Shot 2024-10-14 at 3 09 59 PM

After fix:

Screen Shot 2024-10-14 at 3 07 18 PM

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have checked no similar existing pull requests exist
  • I have checked the CONTRIBUTING document
  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

set `--gradient-background-colour` var instead of `background` property
@@ -18,7 +18,11 @@ See this project's [releases](/../../../releases).
<!-- ### Security -->
<!-- ### Notes -->

## [Unreleased]
Copy link
Author

Choose a reason for hiding this comment

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

Removed this [Unreleased] heading since it looks like all the below changes are already released

Copy link
Owner

Choose a reason for hiding this comment

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

Hiya sorry, this title is of equal level to the version titles below and is empty as I haven't locally developed any changes that are yet to be released. But if for example, I added a new feature X and pushed to github, I'd note it here and then when the new version of the plugin is released, this would automatically get set as the new version title. i.e. Since your changes aren't released as a new version yet, I would put them under 'Unreleased' and when a new version is pushed they would automatically be listed under the latest version. Does that make sense? (Not sure this is the best method since this is the only repo i have in any level of 'production' status but it's worked for me so far so if that's not traditionally what's done, always happy to update).

Copy link
Author

Choose a reason for hiding this comment

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

Ohh gotcha that makes sense! I'd missed the section above where it's mentioned that the changelog is based on Keep a Changelog.

I can think of a couple slightly less ambiguous approaches to this (and e.g. Common Changelog avoids the Unreleased section entirely for a couple reasons), but since this is already following Keep a Changelog, it's probably better to keep it as-is than to diverge from that documented format.

Reverted back!

@mayurankv
Copy link
Owner

Hi! Thanks for the fix! I need to do some quick recap to see whether this fix has no repurcussions and to see if there was a good reason the previous css line was there. If not, I'm happy with the fix. However, could I request that you revert the Unreleased change (or suggest an alternative to the workflow I described in the other comment) before I merge this?

@@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

See this project's [releases](/../../../releases).
See this project's [releases](https://github.com/mayurankv/Obsidian-Code-Styler/releases).
Copy link
Author

Choose a reason for hiding this comment

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

While we're at it - this was a broken link (missing the repo part of the URL), so I went ahead and fixed it with an absolute link if that sounds good to you

Copy link
Author

Choose a reason for hiding this comment

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

Btw, I also noticed that manifest.json has a broken Buy Me A Coffee link. If you want I can also go ahead and add that quick fix to this PR (or separate PR if you'd prefer).

@alythobani alythobani requested a review from mayurankv October 15, 2024 18:07
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.

Bug: Active line not highlighted when codeblock language is specified
2 participants