Skip to content

lineNumber option #19

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

Merged
merged 4 commits into from
Feb 7, 2020
Merged

lineNumber option #19

merged 4 commits into from
Feb 7, 2020

Conversation

hasan-ozbey
Copy link
Contributor

@hasan-ozbey hasan-ozbey commented Feb 7, 2020

Adds lineNumber option for HTML renderers.

I'm building a Flarum extension and line numbers ruins the display in a modal.

@jfcherng
Copy link
Owner

jfcherng commented Feb 7, 2020

ruins the display in a modal

how?


I would prefer this being done via CSS from the user-side since I consider this as a rare situation.

For example,

.diff.diff-inline thead th:nth-child(1),
.diff.diff-inline thead th:nth-child(2),
.diff.diff-inline tbody tr[data-type] > th:nth-child(1),
.diff.diff-inline tbody tr[data-type] > th:nth-child(2) {
    display: none;
}

Mmm... the side-by-side one seems not able to be done like the above one. But if your issue comes from a narrow modal, I think that side-by-side is the widest layout and will be never working as intended?

@jfcherng
Copy link
Owner

jfcherng commented Feb 7, 2020

I have fixed phan errors in v6, please rebase or merge. There are still PSR-12 issues but they can be fixed by $ composer fix easily.

@jfcherng jfcherng added the enhancement New feature or request label Feb 7, 2020
@hasan-ozbey
Copy link
Contributor Author

hasan-ozbey commented Feb 7, 2020

wider modal is not a solution because there are mostly small changes in a forum environment also line numbers in a table is not good for a responsive design. I'd prefer a CSS option for this too, maybe we can add new class names for <th> nodes which shows up with Line Numbers?

@jfcherng
Copy link
Owner

jfcherng commented Feb 7, 2020

But I think pure CSS solution cannot cover the colspan problem in the SideBySide renderer.

return
'<thead>' .
'<tr>' .
'<th colspan="2">' . $this->_('old_version') . '</th>' .
'<th colspan="2">' . $this->_('new_version') . '</th>' .
'</tr>' .
'</thead>';

Another solution without add an extra option is creating something like InlineNoLineNumber renderer.

@hasan-ozbey
Copy link
Contributor Author

hasan-ozbey commented Feb 7, 2020

Another solution could be removing colspan attributes from the PHP and define them within the CSS using new class names of <th> nodes. I think a new renderer is a bit too much for this.

@jfcherng
Copy link
Owner

jfcherng commented Feb 7, 2020

Another solution could be removing colspan attributes from the PHP and define them within the CSS using new class names of nodes

Afaik, there is no colspan equivalent in CSS?

@hasan-ozbey
Copy link
Contributor Author

Whoops! You're right, i completely forgot it. Let's think about a CSS solution little more --

@jfcherng jfcherng merged commit 69f42a1 into jfcherng:v6 Feb 7, 2020
@jfcherng
Copy link
Owner

jfcherng commented Feb 7, 2020

Thanks. Added in 6.4.0.

@jfcherng
Copy link
Owner

jfcherng commented Feb 7, 2020

Changing HTML renderers' output to

  • Be responsive
  • Use CSS for show/hide line numbers (or other features)

could be BC break I guess.

People may discuss and implement in v7 if that ever happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants