a few button, border, and icon color refinements

[zekefarwell]

A something still wasn't looking quite right about some of the contrast and line weights so I looked closely and the old css through the wayback machine and made some more refinements to match the classic style even more closely.

• Slightly darker icons on watch, star, and fork buttons at the top of page
• Slightly darker border on counter boxes next to them
• Slightly darker borders on form text fields
• Slightly darker borders on Box-headers (match the rest of the Box)
• Remove subtle box-shadow from buttons that didn't exist in classic style
• Better blending of button border and background

All the css rules are taken straight from a recent wayback machine copy of github. These changes are very subtle, but I feel they bring back just a little more contrast and polish to the overall look.

Before

After

[daattali]

I did notice all these things too, except for "Better blending of button border and background" - that I don't see the difference. I believe you though, I just don't notice it!

Since we're in this PR, let me show you the one related thing that's been bothering me: the active (clicked) state of buttons. In some buttons I managed to get the active state to look right with the inner box shadow, but on some buttons it uses the new style of just plain gray. For example, in a repo that you own:

• If you click and hold "Add file", you'll get the inner box shadow.
• If you click and hold "Go to file", the background will just be light gray
• If you click and hold "Branch: master" the background will be gray, but if you release the click and let the dropdown be open, then the inner box shadow shows

Think that's something to address in this PR?

[zekefarwell]

Oh yeah look a that. I hadn't noticed those inconsistencies. Also chrome and firefox seem to have slightly different behavior. I'll see if I can fix it and push up another commit to this PR.

[zekefarwell]

except for "Better blending of button border and background" - that I don't see the difference. I believe you though, I just don't notice it!

Yeah this one I barely notice on the gray buttons but I think it does improve the look of the primary (green) buttons. It's three background properties that cause the background gradient of the button to overflow behind the border. Because the border is a transparent gray it then takes on the color of the button. It also takes on the shading of the gradient making the border lighter on top and darker on the bottom. It's a neat technique that I hadn't seen before.

[zekefarwell]

I was able to make the "Add file" button keep it's inner shadow while the dropdown menu is open (same as the branch button/menu). The issue where the inner shadow doesn't show up at all on click is weird though. I think it will need some deeper digging.

When I click the "Go to file" button, there is no inner shadow, but when I force the active state in dev tools, the inner shadow shows up. The inner shadow also shows up on click when javascript is turned off. So it seems something about github's JS is preventing the active state. In Firefox that is the only button I've noticed this issue on, but In chrome I'm noticing it on lots of buttons: the top right buttons (sponsor, watch, star, fork) and many others. The behavior is the same: no inner shadow on click but forcing :active in dev tools shows inner shadow. Turn off JS and all of a sudden inner shadow shows on click.

So I'm not sure what's going on with that. I think that should be solved in a separate PR.

[zekefarwell]

Also, I just found github's styleguide for the old look and feel. Could be quite helpful for UI elements that only show up on private pages like settings so we can't see them on the wayback machine. Here's the buttons page:

• https://styleguide.github.com/primer/components/buttons/

[daattali]

The "Add file" button should not retain the box shadow background when clicked. Its dropdown works a bit differently from the "Branch:master" and the Clone buttons, it doesn't stay clicked in. Don't worry about this too much though :)

[zekefarwell]

I can't see the "Add file" button the wayback machine (seems like it's either new or only shows up when signed in), so maybe you're right. Not sure why it would be different from every other button that opens a dropdown/popout menu and retains it's clicked in state. Seems like it's this component from the old styleguide, and it shows a clicked in state when the menu is open: https://styleguide.github.com/primer/components/dropdown/#basic-examples

Anyway, I hadn't even noticed it before you mentioned the active state issues. Not too concerned either way.

[daattali]

Hmm let's settle on "it's unclear". I don't actually know how that button clicked state was before the updaet. What I said was based off the current UI - where the Clone and Branch buttons do a clicked state when you leave them, but the Add File button does not. But it's also very possible that github just messed up with that button, I don't see why they would make that one different...

[daattali]

Is this PR ready?

[zekefarwell]

I would have said yes an hour ago. But I just noticed a little issue and pushed a fix. I also gave some more thought to why those buttons aren't responding to clicks as expected with the inset shadow and I think I've figured out the issue! I'll push up another commit shortly and then I think it will be ready.

Also would you like me to revert the change that makes the "Add file" button keep its inset shadow while the menu is open or leave it?

[zekefarwell]

Ok this PR should be ready unless there is anything else you'd like to see changed. Sorry for all the extra commits. The issue with the active state inset shadow not showing up is caused by a rule in github's stylesheet targeting the button :focus state that removes all box shadows. That rule is more specific than the rule we are using to target the button :active state and since buttons are often in :active and :focus state at the same time, the shadow got removed. I've added a more specific rule to override this. I think adding !important might also take care of it but figured we might want to avoid that if possible.

[zekefarwell]

Just kidding, one more fix was needed. Now it should be ready.

[daattali]

Regarding "Add File" button: since we agreed that it's unclear what the original look was, I think it's fair for me to say that either one is fine. I don't have a strong opinion on that one.

I'm not home right now, will merge when I get a chance to test.

[daattali]

Testing right now. I thought that you somehow completely hanged the "Clone" button, but looks like github just did another update today

[daattali]

I've found one regression bug: The danger zone in settings should have a red outline, but it looks like 3 of the 4 sides are being overwritten to grey, and the border-radius doesn't seem to be right there

[zekefarwell]

The danger zone in settings should have a red outline, but it looks like 3 of the 4 sides are being overwritten to grey, and the border-radius doesn't seem to be right there

Looks like that happened in my last PR. Sorry for not catching that. Want me to add a fix on here, or handle separately?

[daattali]

This PR here also addresses some box borders, so it can be done here

On Wed., Jul. 1, 2020, 18:29 Zeke Farwell, notifications@github.com wrote:

The danger zone in settings should have a red outline, but it looks like 3 of the 4 sides are being overwritten to grey, and the border-radius doesn't seem to be right there

Looks like that happened in my last PR. Sorry for not catching that. Want me to add a fix on here, or handle separately?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/daattali/oldschool-github-extension/pull/28#issuecomment-652677342, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHIQFBJYML2TWENOIPPGTDRZO2DRANCNFSM4ONAWEJA .

[zekefarwell]

ok just pushed up a commit that should fix the danger zone border issues

[daattali]

The danger zone box still has a tiny tiny UI issue - the top left and top right corners seem to have both 3 and 6 px radii so it looks a bit off. It's very slight, but if anybody would notice it other than me, it's you :)

[zekefarwell]

Hmm this change should have fixed that. The first row within that box also has a border for some reason and it had a border radius of 6px while the overall box had a border radius of 3px. Looks like this for me in Firefox and Chrome:

Are you seeing something different, or am I just missing what the issue is?

[daattali]

Ok I see what it is. It's so miniscule that I don't think the human eye is supposed to even notice this.

Here's what I see

The border radii are indeed 3px on both the box and the row, that isn't the issue, I was wrong. The issue is simply having a border on both. If I remove the border from the Box-row, this is how it looks

I know, pretty much identical, the first one is just ever so slightly thicker. I'm ok with this :p

[zekefarwell]

Oh wow you're absolutely right, and it does look better with that extra top border removed. No idea why that's there but it's definitely present without old school github enabled. We could set it to border-color: transparent; in a future change and it would look a little better. I also just noticed a different danger zone box on the organization settings page isn't being targeted by the same css that changes the border radius from 6px to 3px 😄

[daattali]

Why is github making us jump through all these hoops. It would be so kind of them to name similar elements with similar CSS classes.

[zekefarwell]

Since this PR is still open I went ahead and pushed up another commit that hides that duplicate top border on the danger box, and fixes the other 6px border radius boxes I found. Turns out the rules we had for setting box radius to 3px were targeting .repository-content .Box and the repository-content class isn't present on all pages. Changing it to body .Box appears to do a better job.

Anything else you want me to change?

[daattali]

No, all seems good! I'll review when I find time tomorrow