Filename overflows file header in PRs

[auscompgeek]

Looks like this was caused by 9b5fa7e.

• Browser: Firefox 61.0.1
• Operating System: Arch Linux
• URL: Add fluent builders for more flexibly adding data to Shuffleboard wpilibsuite/allwpilib#1022
• Screenshot:
  
• HTML of the section where the issue occurs:

<summary class="file-header js-toggle-outdated-comments">
    <span class="btn-link text-gray float-right f6 outdated-comment-label show-outdated-button"><svg class="octicon octicon-unfold position-relative mr-1" viewBox="0 0 14 16" version="1.1" width="14" height="16" aria-hidden="true"><path fill-rule="evenodd" d="M11.5 7.5L14 10c0 .55-.45 1-1 1H9v-1h3.5l-2-2h-7l-2 2H5v1H1c-.55 0-1-.45-1-1l2.5-2.5L0 5c0-.55.45-1 1-1h4v1H1.5l2 2h7l2-2H9V4h4c.55 0 1 .45 1 1l-2.5 2.5zM6 6h2V3h2L7 0 4 3h2v3zm2 3H6v3H4l3 3 3-3H8V9z"></path></svg>Show outdated</span>
    <span class="btn-link text-gray float-right f6 outdated-comment-label hide-outdated-button"><svg class="octicon octicon-fold position-relative mr-1" viewBox="0 0 14 16" version="1.1" width="14" height="16" aria-hidden="true"><path fill-rule="evenodd" d="M7 9l3 3H8v3H6v-3H4l3-3zm3-6H8V0H6v3H4l3 3 3-3zm4 2c0-.55-.45-1-1-1h-2.5l-1 1h3l-2 2h-7l-2-2h3l-1-1H1c-.55 0-1 .45-1 1l2.5 2.5L0 10c0 .55.45 1 1 1h2.5l1-1h-3l2-2h7l2 2h-3l1 1H13c.55 0 1-.45 1-1l-2.5-2.5L14 5z"></path></svg>Hide outdated</span>
        <a href="/wpilibsuite/allwpilib/pull/1022/files/7e1216e5fb83bff4153d0a033f5e2edfb1b5a577#diff-9614fb171e262da53ef7674d190ff496" class="file-info link-gray-dark" title="wpilibj/src/main/java/edu/wpi/first/wpilibj/shuffleboard/Shuffleboard.java">
      wpilibj/src/main/java/edu/wpi/first/wpilibj/shuffleboard/Shuffleboard.java
    </a>
  </summary>

[Mottie]

Hi @auscompgeek!

I can't duplicate this problem. I see the .java ending on all three files using the latest user.css style in the same version of Firefox on Windows. I don't have a Linux system to test.

The commit that was shared deals with the "Files changed" tab and shouldn't cause the issue seen in the screen shot.

[auscompgeek]

Hm. Turns out this only becomes a problem if you zoom in. (I always have GitHub zoomed at 110% because the code font size is oddly small to me otherwise.)

It's definitely caused by the CSS introduced in that commit though; the CSS selector .pull-request-tab-content .file-header does select those headers.

[Mottie]

LOL, I'm always zoomed in to 110% as well. I'm still not seeing the problem. What happens if you disable the padding: 1px 10px !important definition from the dev tools?

[auscompgeek]

No dice.

If it helps, it seems my file headers are using the Droid Sans Mono Slashed font, and the sans-serif font is Noto Sans.

[the-j0k3r]

Looks like this was caused by 9b5fa7e.

You are poking at the right place its the height: 32px !important; portion in.

    .pull-request-tab-content .file-header {
      height: 32px !important;
      padding: 1px 10px !important;
    }

See this.

So you see when you disable GitHub Dark the .discussion-item-review .file-header is much taller to accommodate overflow so the issue is elsewhere.

A possible solution is adding :not(summaryin that 9b5fa7e block

    .pull-request-tab-content .file-header:not(summary) {
      height: 32px !important;
      padding: 1px 10px !important;
    }

Excluding the summary does the trick here, but it may be too broad. I need to do more testing to find something more targeted..

[the-j0k3r]

IDK, so far both :not(.js-toggle-outdated-comments) and :not(summary) are the only ones that work in that position.

And neither of those affect the functionality of 9b5fa7e or the area that it proposes to fix here

edit: however the same problem exists when looking at https://github.com/wpilibsuite/allwpilib/pull/1022/files#diff-b0bc4c1c897b5abdb841f75a8f7896f7R3

The height block screws with that also in the same way this issue is affected.

So how to fix both and keep 9b5fa7e is the question. Back to the drawing board.

[the-j0k3r]

OK got it, fix incoming.

[auscompgeek]

Heh, I was wondering whether the height rule there was necessary.

[the-j0k3r]

Its more complicated that that see #598

which is why that was developed. But because the heigh can increase in file-header to accommodate extra long lines the issue with those re-happens again in these portions.

[the-j0k3r]

@auscompgeek see #598 (comment) it explain the issue further.

[Mottie]

Maybe we should add the ".diff-view" in the middle of those selectors:

.pull-request-tab-content .diff-view .file-header {
  height: 32px !important;
  padding: 1px 10px !important;
}

That way it only targets the files changed tab.

[the-j0k3r]

@Mottie no, because those headers also have double height. which was restricted by that and thus this issue also affected the diff view file header which no longer is the case. read the comment above yours ;)

[Mottie]

GitHub truncates the path/filename so that it doesn't expand into two rows. Try the CSS I shared, it won't effect the file names in the conversation tab.

[the-j0k3r]

Again no this issue affects two portions what @auscompgeek pointed out and the fileheaders in diff view.
Disable GHD and look not just at this but also at the info I posted at #598 (comment)

If you dont want to, just look at this with the above code.

[Mottie]

Ok, I still can't reproduce, so I'll leave it to you.

[xt0rted]

I can't repro this either. Out of curiosity what font size are you seeing used on that text? Mine's 12px and the only way I can get the text to overflow is to increase the size.

[the-j0k3r]

this

    .pull-request-tab-content .diff-view .file-header {
  height: 32px !important;
  padding: 1px 10px !important;
}

equals

font size or no font size the difference between default and GHD is painfully obvious

[the-j0k3r]

This maybe a Firefox thing I realize neither of you use Firefox but the full width of elements is cut off here and these headers overflow like showed above.

[xt0rted]

This is what I see with FF 62

[the-j0k3r]

Only testing stable release here at this time

[Mottie]

I did test this issue in Firefox. I'm not seeing the overflow of the file into the second line. This is why we shouldn't be adding padding or other non-color styling unless it is absolutely necessary.

[the-j0k3r]

It is necessary :(

[Mottie]

Maybe that line needs a white-space: pre, or something set on it so it won't wrap... GitHub added the ellipsis to the file name because they didn't want it to wrap.

[the-j0k3r]

It wont work whitespace pre.

Ive played with this to ad nauseaum to fix #598 and whatever tweaks are there are necessary to make it work here

Are you even looking at the gifs Im posting? anyway, Do whatever, Ill just open bug reports or fix it localy

[the-j0k3r]

white space pre

[Mottie]

Actually, I was thinking about this:

.pull-request-tab-content .file-header {
  text-overflow: ellipsis;
  overflow: hidden;
  white-space: nowrap;
}

[the-j0k3r]

You cant remove the padding else what that proposes to fix is null and void, but lets ignore that for a second.

Your proposal.

Text is now overlapping and as you can see when I disabled GHD the header is double height.

[xt0rted]

In your settings do you have a minimum font size set? Because If i set that to 14px then I'm able to repro the overflowing text.

If I disable the 32px file header rule, with and without Refined GitHub enabled, the headers expand when needed and look to work fine in both the comments view and the diff view. Why is this height being explicitly set in the first place? Was it just a stylistic choice like how the diff header is shorter in GHD than by default?

[the-j0k3r]

In your settings do you have a minimum font size set?

I do, but does that explain the differences between default and GHD?

Edit: I actually disabled that, and result is I cant read the text on my screen without shoving my head with my nose two inches away from screen 27" screen at 2560x1440p

So not an option.

[xt0rted]

It explains why the text is overflowing and why we couldn't see it. Now @Mottie can adjust his settings and see it too which is better than a screen shot.

[the-j0k3r]

Sucks to be half blind in a modern world of UHD

[Mottie]

I did adjust the font size... that's how I came up with #724 (comment)... I'm done for tonight. You guys hash it out.

[the-j0k3r]

I'm done for tonight. You guys hash it out.

Im also done tonight and with this

Ive had an extremely long day and cantdiscuss the reasons why 9b5fa7e has to be as it is since it was already itself a long discussion in #598 and #612 and since its inception in #690 and after the fact in 5743cd7 why the reason of each value existence discussing it further is just going nowhere.

This issue is fixed and so is 50% for me of #598 which is a minor miracle considering I would like to see the text on my screen.

That said, if anyone has a better proposal that fixes everything and is a decent compromise, feel free to leave the code and Ill test it.

I did adjust the font size... that's how I came up with #724 (comment)...

That overlaps at the right edge here so Im definitely lost in translation ;)

[silverwind]

Instead of setting a minimum font size which ought to break layouts, I can recommend a extension like this where you can set a default zoom level. I usually use 200% on screens above 1080p.

[the-j0k3r]

I dont like distorting things too big font is not good either with zoom levels so everything is 100% at 100% DPI, I could have also opted to use higher DPI but that is more distortion OS Side too, neither look as natural as my current setup which has taken a long time to mature in most text based sites.

This is my best result sadly. Things break equally in some sites with zoom levels or DPI, this works best across all I need.

I could lower my resolution, but that defeats purpose for having a larger screen where I can dump VM + browser + some other bits which is why I bought a bigger higher resolution screen and why I keep it at 2560x1440p and not the full 4K native resolution 3840x2160p because then I would really need 200% everything and everything is out of wack.

I dont play games so no good to me.

• default GH style handles my setup nicely as testified by screenshots, and there is no reason why GHD cant, or is there some reason not to?