Skip to content

add option showHeader #106

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

Closed
wants to merge 3 commits into from
Closed

add option showHeader #106

wants to merge 3 commits into from

Conversation

vendeeglobe
Copy link

HTML Inline Renderer -> Options

Name Type Default Description
showHeader bool true Generation of the table header.

https://github.com/JBlond/php-diff/wiki/2.-Parameters-and-Options

false disables the generation of the table header in the HTML renderers
e.g. $renderer = new SideBySide(['showHeader' => false]);
@DigiLive
Copy link
Collaborator

@vendeeglobe Thanks for your effort.

  1. jblond\Diff\Renderer\Html\Merged has method generateDiffHeader() also.
    Shouldn't the changes be included into this class also?
  2. Maybe the option should be refactored to something like addTableHeader since code is generated, but not shown by the class.
  3. We shouldn't forget to update the Wiki once merged.

@JBlond Did you read my email?

@JBlond
Copy link
Owner

JBlond commented Oct 29, 2021

I don't like this change. In the discussion You (@vendeeglobe) mentioned jfcherng/php-diff@5f0dd2d
There is a clean stop

if (!$this->options['showHeader']) {
            return '';
        }

This patch on the other hand only tries to change some of the code. It is a cleaner option to extend the desired class and override a single method. See https://github.com/JBlond/php-diff/wiki/3.-Custom-Renderer

@@ -72,15 +74,22 @@ public function render()
*/
public function generateDiffHeader(): string
{
return <<<HTML
$html = <<<HTML
Copy link
Owner

Choose a reason for hiding this comment

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

This is not a clean solution.

@DigiLive
Copy link
Collaborator

DigiLive commented Nov 1, 2021

JBlond wrote:

I don't like this change.

I vote for what I offered at issue #53 (comment)

IMHO overriding is the cleanest and most versatile way to generate custom output, based on a current renderer.
I'm not saying the request of vendeeglobe is invalid, but It's unmanageable for us to implement each and every requests for custom output.

However, I do appreciate his usage of our package and the feedback he gives us.
I'm willing to help him to implement the suggested overrides.
We can discuss it at https://github.com/JBlond/php-diff/discussions if he pleases so.

@JBlond
Copy link
Owner

JBlond commented Nov 2, 2021

@vendeeglobe is that an option for you that DigiLive helps you to make that changes in your repo?

@JBlond
Copy link
Owner

JBlond commented Nov 16, 2021

There wasn't any feedback within two weeks. Please add another commend, if you want to contiune.

@JBlond JBlond closed this Nov 16, 2021
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.

3 participants