Skip to content

Conversation

@kidonng
Copy link
Contributor

@kidonng kidonng commented Jul 16, 2022

I just found this script and it works like charm! I mean, without other extensions in the way...

This pull request fixes some compatibility issues with other browser extensions:

  • Refined GitHub's quick-file-edit feature, which is a one line fix
  • File icon extensions (in general), which also brings some style change

Now icons look good in dark mode:

image

With https://github.com/Claudiohbsantos/github-material-icons-extension:

image

I'm aiming for a minimal diff in case you want to argue about e.g. the variable name a little bit.

// not every submodule includes a link; reference examples from
// see https://github.com/electron/electron/tree/v1.1.1/vendor
const el = $("a", row) || $("div[role='rowheader'] span[title]", row);
const el = $("div[role='rowheader'] a, div[role='rowheader'] span[title]", row);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the one line fix for Refined GitHub

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks!

Comment on lines -231 to -239
const str = url.substring(url.lastIndexOf("/") + 1, url.length);
// don't include extension for folders, or files with no extension,
// or files starting with a "." (e.g. ".gitignore")
content += (!noExt && str.indexOf(".") > 0) ?
"<h4 class='ghip-file-type'>" +
str
.substring(str.lastIndexOf(".") + 1, str.length)
.toUpperCase() +
"</h4></span>" : "</span>";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not look good with file icon extensions, so I dropped it. However, it makes it a little worse for those who don't use them. I believe the tradeoff is worth it, let me know your thoughts.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I think you're right.. I'm going to go ahead and merge your work, but I'm going to tweak it a bit to show the full file name. Thanks again!

@Mottie Mottie merged commit bc9181c into Mottie:master Jul 17, 2022
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.

2 participants