material-extensions / material-icons-browser-extension

Browser Addon that enhances file browsers of version controls with material icons.
MIT License
499 stars 38 forks source link

Doubled icons when used with Refined GitHub #65

Closed Milo123459 closed 1 year ago

Milo123459 commented 1 year ago
image

found on this repository by clicking on code

cobrabr commented 1 year ago

Same here. Started happening just now, it was fine a few hours ago.

AdamRaichu commented 1 year ago

I don't see this (v1.8.0).

Edit: this is because I had already disabled rgh's quick-file-edit when using https://github.com/Claudiohbsantos/github-material-icons-extension/issues/55#issuecomment-1507342774

Xithrius commented 1 year ago

I have this same issue in Mozilla Firefox version 112.0.2, on extension version 1.8.1

anaclumos commented 1 year ago

Same

image
Xithrius commented 1 year ago

An issue on the refined-github repository describes this very problem. According to said issue, a solution is to disable quick-file-edit in the extension settings of refined github.

Milo123459 commented 1 year ago

according to the maintainer, this is something that should be fixed in this extension, so I guess this issue should remain open?

cobrabr commented 1 year ago

according to the maintainer, this is something that should be fixed in this extension, so I guess this issue should remain open?

Agreed.

csandman commented 1 year ago

@Claudiohbsantos I took a look and it definitely has to do with how my changes are prepending the icon instead of replacing the original one. From what I can tell, RGH is actually wrapping both this extension's icon, as well as the original icon, in a link and adding the pencil icon after them. If you hover an icon from this package, the RGH pencil is hidden by the styles I added in my last PR to hide GitHub's original icon.

I'm not sure exactly how RGH is targeting the icons for wrapping, but I assume it's something similar to what this package does, as the class names from the original icon are added to the ones from this package (which is why I assume they're both getting targeted by RGH).

Anyway, I'm not sure if there is a great solution to this without fundamentally changing the code I implemented for the icon replacement in the new code view. One possibility could be to only do the prepending stuff if the icons are actually in the new code view's sidebar, but that might get complicated.


Anyway, I'll leave this one up to you. I don't think people should necessarily expect two different extensions that modify the same website to play nicely together haha.

wajeht commented 1 year ago

same here!

owocado commented 1 year ago

I am also observing this same issue on Google Chrome Version 113.0.5672.63 (Official Build) (64-bit) with extension version 1.8.0

Edit: the workaround posted by @AdamRaichu and @Xithrius above worked for me, many thanks ❤️

Claudiohbsantos commented 1 year ago

Yeah, let's keep this issue open here since RGH closed theirs to see if we can get both extensions to play along nicely.

@csandman Yeah, I was poking around the DOM with both extensions and I see what you mean. I couldn't quite pinpoint why RGH is wrapping both elements, it must have to do with how we copy all attributes when creating our copy. I'm hoping there's a workaround where we can override their CSS to force things to play nicely, but then we'll start playing catch up with both github itself and RGH in terms of DOM structure, which is definitely not ideal.

Agreed that this might be hard to completely prevent. I'll still keep the current changes for now since I think the new code view is still a valuable addition.

AdamRaichu commented 1 year ago

Suggestion: change the title to be more descriptive (maybe include Refined GitHub. For example, Doubled icons when used with Refined GitHub.

csandman commented 1 year ago

I couldn't quite pinpoint why RGH is wrapping both elements, it must have to do with how we copy all attributes when creating our copy.

Yeah, it's based on their selectors, I just checked the source code:

    observe([
        '.react-directory-filename-column svg.color-fg-muted',
        '.js-navigation-container .octicon-file',
    ], linkifyIcon, {signal});

So if we break down the issue, the ideal solution would be for only this package's custom icon to be wrapped, for the pencil icon to be visible when you hover it instead of hidden, and for the original icon to remain hidden. There are a few problems about fixing this, as I see it.

  1. RGH's function wraps the original icon, which results in the hiding css from this extension to not be applied to the original icon anymore. This is what is causing the bug actually mentioned here, that there are now two icons. This could potentially be prevented by removing the selectors from the original icon that RGH is using to select them, but there is a possibility that would cause other problems.
  2. Even if the original icon wasn't wrapped (and the one from this package was) the CSS for hiding the original icon would no longer function, as it's based on the custom icon and the original being direct siblings.
  3. If we say we want the custom icon to be wrapped, the CSS used to hide the original icon would then apply to the pencil icon, which is not what we want.

All in all, I can't see an obvious way to fix this without completely changing the way the GH selectors in this package work. The reason for switching to an append approach instead of a replacement is that the new File Tree view completely explodes if the original icons aren't there to be manipulated by GH's react code. So if the replacement logic is changed, it needs to be done in a way where the original icon is still there.

As I was writing this, I was thinking about some way to append the new icon as a child of the original SVG, but I don't think there's any valid way of doing that. One strategy that might be possible, is hiding the original icon by deleting the SVG path that's inside it, and adding the custom icon as a background image to it, but I have absolutely no idea if that would work.

Anyway, it's a tricky problem haha.

fregante commented 1 year ago

https://github.com/homerchen19/github-file-icons is natively compatible with Refined GitHub, so it should be possible to have both. There are plenty other icon extensions that are compatible.

fregante commented 1 year ago

I sent a PR with essentially +1 line of code:

Claudiohbsantos commented 1 year ago

@fregante thank you very much for taking a look at compatibility here and opening that MR. I have just merged it and have tested it locally with success!

csandman commented 1 year ago

I sent a PR with essentially +1 line of code:

Guess my "it's a tricky problem" was entirely incorrect 😂 glad to see it fixed!

fregante commented 1 year ago

I sent a PR with essentially +1 line of code:

My solution was actually slightly incorrect actually 😅 I added more info to the PR

Milo123459 commented 1 year ago

1.8.2 isn't on the chrome web store :(