daattali / oldschool-github-extension

Revert GitHub's UI back to its classic look (before the June 23, 2020 update that has a flat, rounded and more whitespaced design).
https://chrome.google.com/webstore/detail/old-school-github/blkkkhifjoiedclojflfcenbjigdajeb
MIT License
229 stars 11 forks source link

Revert border-radius to 3px for buttons, labels, and nav elements #22

Closed zekefarwell closed 4 years ago

zekefarwell commented 4 years ago

The PR reverts buttons, nav elements, and labels to a 3px border radius like they were in the classic UI. I tested quite thoroughly looking for button types that should not have a radius on all corners (split buttons, input groups, etc). I think I've got them all excluded, but it's certainly possible there are more hiding in parts of github I didn't manage to browse to.

Partially addresses #19 but still plenty more 6px corners to deal with.

A couple fixes:

daattali commented 4 years ago

Oh no, it's spreading!

Can you make the gist.github.com a separate PR, I want to keep the history clean and easier to browse through

zekefarwell commented 4 years ago

Happy to do a separate PR for that yeah. Just need to figure out how to organize two PRs at once. I guess I just make two separate branches in my forked repo? or do I need two separate forks?

daattali commented 4 years ago

I think two branches would work. Or if you just request to edit the manifest directly on github, I think it'll automatically make that a new PR.

Curious - which button had outline issues on hover?

daattali commented 4 years ago

Nevermind, I see it! The "browse files" button

zekefarwell commented 4 years ago

I've updated this commit to revert the gist.github.com change. Would you rather I delete this PR and open another without that change at all, or do you intend to to squash the commits somehow?

daattali commented 4 years ago

You can keep this PR. I do a squahs merge, so the ugly history won't show.

zekefarwell commented 4 years ago

Yeah the browse files button, and it shows up in another spot I'd never seen before where you can look at just specific commit from a PR and then a previous|next button shows up.

daattali commented 4 years ago
  It's possible that setting all button border radiuses to 3px and then overriding where they
  should be square would be a better approach.
*/
:not(.input-group-button) > :not(.BtnGroup):not(.subnav-search-context):not(.input-group-button)
> .btn:not(.BtnGroup-item):not(.btn-with-count):not(.get-repo-btn) {

Would it be a pain to do that? To make everythin g3 by default nad specifically target the exceptions for squareness? It might be mor emanageable CSS?

zekefarwell commented 4 years ago

Yeah I'm not sure if it would be more manageable or not. I started adding the exclusions because there are quite a few button groups (split buttons) where the inner corners should not have round corners at all. So if we apply the 3px as the default, then we have to have to override it to 0px in a whole bunch of places. Maybe that's a more maintainable structure, but I'm not sure since I haven't done it. It could actually be worse!

I added the note because I came across an an input group on gist.github.com where a button was getting a 3px radius on a side that it shouldn't and I was having a hard time overriding it to 0px because of the specificity of this rule with all the chained :not selectors. I ended up making it even more specific so it wouldn't apply to the input-group.

daattali commented 4 years ago

Many new rules are prepended with div while some are body. Can they all use body? I want to try to be consistent, the body I initially chose was just a hacky way to get the rule to be more specific than github's version. If the divs serve the same purpose I prefer to change to body

zekefarwell commented 4 years ago

The divs do serve the same purpose. I used div instead of body just because it's a little more efficient for the browser to not have to traverse the whole DOM tree all the way up to body to satisfy the rule. But I don't think it really makes that much of a difference. Feel free to change them all to body if you prefer that for consistency. I think you should be able to modify the PR since I left the "allow maintainer edits" box checked?

daattali commented 4 years ago

Yes I can. I just wanted to know if they serve a different purpose.

daattali commented 4 years ago

Awesome this look great!

zekefarwell commented 4 years ago

Thanks! And sorry for bombarding you with so many PRs. I just can't seem to stop thinking about how to fix things as I browse github 😄.

daattali commented 4 years ago

Clearly you're not alone in these thoughts. But you're unique in doing something about it :)

I'm moving to a new house today so won't be as responsive

zekefarwell commented 4 years ago

Exciting! Good luck with the move. Please don't ever feel the need to respond quickly. I'm just contributing back changes I'm making for my own satisfaction.

daattali commented 4 years ago

*For the satisfaction of hundreds of people