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

Added support for 'line-no-highlight' class #49

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion includes/class-gistpress.php
Original file line number Diff line number Diff line change
Expand Up @@ -495,12 +495,14 @@ public function process_gist_html( $html, array $args ) {
//$classes[] = ( $key % 2 ) ? 'line-odd' : 'line-even';
$style = '';

if ( isset( $highlight[ $key + 1 ] ) ) {
if ( isset( $highlight[$key + 1] ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When square brackets contain a variable, the extra spaces should be preserved.

Copy link
Author

Choose a reason for hiding this comment

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

I can't maintain this within my IDE. The auto-formatting options in PHPStorm are simply have a space or don't have a space within brackets – there is no condition based on whether or not the content of brackets will be a variable, so I can never really accurately follow that particular piece of Wordpress coding standards. I'm wrong either way.

I'm just using the PHPStorm config settings maintained by Rarst - https://gist.github.com/Rarst/1370155

$classes[] = 'line-highlight';

if ( ! empty( $args['highlight_color'] ) ) {
$style = ' style="background-color: ' . $args['highlight_color'] . ' !important"';
}
} elseif ( ! empty( $args['highlight'] ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of checking elseif, would just else work here instead?

Copy link
Author

Choose a reason for hiding this comment

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

Changing the elseif to else negates the need for the class altogether, as the line-no-highlight class is only required when there is highlighting within the gist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's been a while since I worked on this, and @bradyvercher might correct me, but the opening condition of:

if ( isset( $highlight[ $key + 1 ] ) ) {

...says that "if highlighting is to be applied to this line, then...", which means the else translates to "no highlighting should be applied to this line".

Hang on. I'm going on the assumption that you're after the following.


Some highlighting:

.line .line-no-highlight
.line .line-no-highlight
.line .line-no-highlight
.line .line-highlight
.line .line-highlight
.line .line-no-highlight

No highlighting:

.line .line-no-highlight
.line .line-no-highlight
.line .line-no-highlight
.line .line-no-highlight
.line .line-no-highlight
.line .line-no-highlight


But are you actually after:


Some highlighting:

.line .line-no-highlight
.line .line-no-highlight
.line .line-no-highlight
.line .line-highlight
.line .line-highlight
.line .line-no-highlight

No highlighting:

.line
.line
.line
.line
.line
.line


? Only adding the no-highlighting if there is at least one line which does have highlighting? In which case, what you've got would be correct.

Copy link
Author

Choose a reason for hiding this comment

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

That's the goal.

$classes[] = 'line-no-highlight';
}

/**
Expand Down