Icon size improvement

[christosnc]

Hello,

This is a very nice extension! I tried it and I immediately noticed an issue with the icons being too small compared to the original ones.

I added the following in main.js (after const newSVG = document.createElement("img");):

newSVG.style.width = "20px";
newSVG.style.height = "20px";
newSVG.style.marginTop = "1px";
newSVG.style.marginBottom = "-1px";

This makes it so much better IMO. Please try it out / consider this.

[Claudiohbsantos]

Hey @christosnc . Thanks for the suggestion and the code , I tried it out and I do indeed like the slight increase. I am a bit afraid of increasing the size though because all users are likely already used to the current size and a sudden increase might be the kind of change that is weird to people who have been using the extension for a long time.

Let me think about this for a second and see if I can come up with a smooth way to allow for this change or if I'm just overthinking it.

Sorry about the delay as well, I know it's been a while!

[elijaholmos]

Hi, I would be interested in seeing this implemented. Perhaps there could be an extension setting that allows users to select the icon size from a list of options, "small", "medium", and "large". The current icon size that many users are used to could be the default size and maybe the "small" option. @christosnc's proposed adjustment could be "medium".

[Claudiohbsantos]

@elijaholmos That's a good idea. I'm working on an options page where this setting can be selected, along with some other options that would unblock a couple of other issues. My first goal is to fix the auto-release actions (because I'm bound to forget to check the version in the stores again ...) but this is the immediately next priority.

[csandman]

I generally agree that making them bigger would be a nice improvement! Even just 18x18px looks a bit better. There are two things to keep in mind though.

• Different Git providers use different default sizes (Bitbucket icons are large by default), so if you added an option for changing the sizes you'd need to make sure the custom sizes make sense for each site they were being applied to
• Generally this extension is good about replacing the icons before the first render, but in the situations where it doesn't, it can be a weird look if the icon is so big that it either pushes the text over after the first render, or changes the rows height. This extension is an example that uses larger icons that expand the row's height, but only after it first renders the original icons, and its not a great look: https://chrome.google.com/webstore/detail/atom-file-icons-web/pljfkbaipkidhmaljaaakibigbcmmpnc. In most cases this won't be a problem if you don't make them too large, but its definitely something to keep in mind!

Just my 2 cents!

[Claudiohbsantos]

@csandman those are very good points.

Looking at our current providers I've been thinking about defining a multiplier/factor for each provider, and using that to multiply the "size" of the icons chosen by the user. This way we could have a "medium" icon that is slightly different in each provider to ensure it matches the size of the row.

Agreed that the default size should not affect row sizing in any way. That is very jarring

[csandman]

@Claudiohbsantos so I took a stab at this, because it was an interesting challenge (sorry if I'm interacting too much on this repo haha, I've been enjoying it and work has been super slow lately). You can check it out on this fork if you'd like: https://github.com/csandman/github-material-icons-extension/tree/dynamic-sizes

I went off your settingsInterface branch, but that was mostly just to start.

The general idea I went with was adding a CSS stylesheet injected as a content script. I built off of something in my last PR for the other new providers, which was adding a data-material-icons-extension="icon" attribute to all of the icons before adding them to the page. This made it easy to target each of them with CSS selectors, only changing one property on the body to modify the sizes. Also I went off of your multiplier idea using the css transform: scale() attribute which allows you to change the size of the icons without changing the layout of the rest of the page which is nice.

Anyway, if you want to use this, feel free to branch off of it or copy the code. I know you wanted to do something with the icon pack, which this would lay a good foundation for, but you may want to change the structure, popup styles, etc. For one thing, I think it might be good to keep a separate setting for each site the extension is used on, but this was mostly a proof of concept.

Also here's what it looks like:

https://user-images.githubusercontent.com/9214195/177623402-68e0b731-1aea-48ff-9fce-161bf13cd4ae.mp4

[elijaholmos]

@csandman solid proof of concept! the approach you described sounds efficient, nice job.

I agree that different size settings per site would be convenient. perhaps there is a default setting that applies to all sites, then site-specific overrides that could be enabled. i.e. a user prefers the extra large icons on all sites by default (which may be different than the default that is shipped with the extension), so each time this user visits a new site, they would have to enter the settings to change the size from the extension's default to their preferred. if the extension's default icon size could be modified by the user, then this annoying process will be eliminated for this group of users, while also satisfying users who want different icon sizes depending on which site they're visiting.

I definitely wouldn't mind creating a PR to integrate the feature I'm describing, once the proof of concept is built out & merged🙂

[Claudiohbsantos]

@csandman thanks for giving this a shot! And there's no such thing as someone contributing too much here, that's one of the main points of open source after all!

I'll take a proper look into the fork when I have a second but the POC looks great. And I love the idea of handling this entirely in CSS using the attribute we're already (soon) adding to all icons. This should make it very easy to do whatever we want with them without needing to even worry about rendering performance/reloading icons.

@elijaholmos I agree a global default setting might be useful. Let's see how the code ends up as we work on the POC to get it merged and if that doesn't make it into the feature itself I'd be very open for PRs implementing it. I'll try to keep this issue updated with any progress on this feature.

[csandman]

@Claudiohbsantos I also realized that using a shared size across all git provider websites might be totally fine considering most people only really spend most of their time on one site. Just a thought anyway!

[Claudiohbsantos]

@csandman I agree, but I got a tad excited and went all out lol

I just merged a popup based on the branch you started (thanks for that, it was a massive head start) and an options page that allows providers to be customized individually.

@elijaholmos I took a stab at a default setting that is applied to all providers. Providers can have custom settings that override the global default but the default can be easily set as a base preference for a user.

This should all be released on the next store push, which I'm hoping to get out sometime this week.

I'll close this issue since I believe the request is fulfilled by the changes but feel free to tag me here if I missed anything/any follow up discussion comes up.

[csandman]

@Claudiohbsantos I'm glad you got some use out of my branch! The changes look great!

One potential idea that you might want to implement is reloading the page when the icons are enabled/disabled. It's simple enough to do using the chrome.tabs.reload function:

chrome.tabs.query({ active: true, currentWindow: true }, (tabs) => {
  const activeTabId = tabs[0]?.id;
  if (activeTabId !== undefined) {
    chrome.tabs.reload(activeTabId);
  }
});

Up to you, just thought it might simplify things a bit!

[Claudiohbsantos]

@csandman I actually had some similar code in at some point but decided to pull it out because I think unannounced page reloads have the potential to be a pain. For example: You are in the "Go to file" page and have a long complex filter typed that you've been using to narrow down to a specific set of files. You open the extension popup to increase the size of the icon because you want to take a screenshot but accidentaly tap the disable checkbox and the page reload. Boom. Your filter is gone and now you hate me deeply for a couple of seconds :laughing:

I admit the downsides are not that intense and it would be more elegant to automatically change icons but ultimately I decided the best way to do it would be to find a way to restore the icons without reloading the page but that it also wasn't worth the effort for now. Since I don't expect users to frequently toggle the extension it didn't seem like a priority :shrug:

[csandman]

That's a fair point and something I had thought about, but my mind went more to writing our an issue, pr, or comment on one of those two. And I realized GitHub actually saves the state of those text fields when you reload, which would reduce the likelihood of causing problems. But thats Github and I have no idea how the rest of the providers would handle a reload.

I also would generally think that most people wouldn't be changing things while they're in the middle of typing something out. But you're right, it doesn't really offer that much benefit vs the potential pain point if it messes someone's progress up. And yeah, changing the icons back without reloading would of course be the perfect solution, but with the way icons are being replaced right now, that seems like a larger undertaking than its probably worth haha.