search

material-extensions / material-icons-browser-extension

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

Refactor code base for better enhancement and approach #7

Closed shivapoudel closed 3 years ago

shivapoudel commented 3 years ago

Yesterday night I tried to study for the browser extension work and studied this extension as I was interested in the MS Edge. My full efforts for making the codebase clean while understanding, I have also refactored many pieces of stuff to replace the packages we need, my friend :)

Claudiohbsantos commented 3 years ago

@shivapoudel Thanks for opening this PR, I'll take a look as soon as I can (probably later this week).

It seems you are using different indentation settings though, which makes the diff pretty large and makes it harder to review the PR since pretty much every line seems to be changed. Would you mind changing your editorconfig file to match the style of the existing codebase so the PR reflects only actual changes? That would make it much easier to jump into it when I have a chance :)

shivapoudel commented 3 years ago

@Claudiohbsantos Okay will look at it tomorrow :)

shivapoudel commented 3 years ago

@Claudiohbsantos Now you can proceed with the review. I have managed to fix the coding rulesets you have mentioned earlier including fix for svgo error in its latest version which doesn't support --disable in CLI. Additionally, I have chosen a shallow clone of the VSCode extension repo and npm installation without scripts for it to faster the build process of icons for us!

If there is anything missing, please suggest me and I will have a look!!

Claudiohbsantos commented 3 years ago

Thanks for all of these changes, this is looking great. Whenever you feel like it's ready for a full review again just re-request review and I'll jump in whenever I can.

shivapoudel commented 3 years ago

@Claudiohbsantos I am ready with this PR so please give a full review from your side. Additionally, after merging the PR, help with some logic to implement a simple config file for activeIconPack as I will not be looking on that currently with this PR.

shivapoudel commented 3 years ago

@Claudiohbsantos any updates when you can full review this PR, my friend :)

Claudiohbsantos commented 3 years ago

Hey @shivapoudel . Thanks for the work on this. I'm travelling for a quick break until mid next week. I'll make sure to review this asap when I'm back. Sorry for the delay

ghost commented 3 years ago

In the new build scripts, could you add console log comments that give the user an idea of what the scripts currently doing?

I do the same in #10 and it helped me greatly when I was debugging the build scripts 😄

shivapoudel commented 3 years ago

@4086606 I wish I could but unfortunately, I have some other prior tasks which I have to wrap up till the next week. However, that can be done after the merge also so will give a look at that on some other days, my friend :)

Claudiohbsantos commented 3 years ago

@shivapoudel sorry about the delay. I just merged the other PR that preceded this one and will jump on this next. If I can't finish reviewing today I'll do it Monday the latest. Thank you for your patience

Claudiohbsantos commented 3 years ago

If you'd like, I created a PR in your fork with the solution to the merge conflicts flagged here: https://github.com/shivapoudel/github-material-icons-extension/pull/2 . I thought that was easier for you than having to review the PR once again :) , and nicer than pushing the changes directly into this PR. Let me know if anything seems wrong to you.

I'm also happy to merge those changes directly into the PR if that's easier, I already tested the changes and they work as expected

shivapoudel commented 3 years ago

@Claudiohbsantos I have tested the PR raised in the fork and seems nicer. Please proceed :)

shivapoudel commented 3 years ago

@Claudiohbsantos Is downloading Chromium still necessary? image

Claudiohbsantos commented 3 years ago

Oh, i don't think so, thanks for catching that. Looks like I accidentally removed the --ignore-scripts option you added during the merge. I can re-add that after merging

shivapoudel commented 3 years ago

@Claudiohbsantos While running npm run build I am facing this issue with module make-dir we already have mkdirp so can this be the one package that does both works? image

ghost commented 3 years ago

make-dir > mkdirp:

shivapoudel commented 3 years ago

@4086606 Is this okay for you https://github.com/Claudiohbsantos/github-material-icons-extension/pull/7/commits/ebef814f1bad8fcf4d688eaa385dd582c3dc55e9 as for me on Windows now it is working great!!

shivapoudel commented 3 years ago

@Claudiohbsantos Merge conflicts and other issues have been patched. Please have a review and merge the PR :)

ghost commented 3 years ago

Some small issues in Ubuntu (WSL), will open a pull to fix those

Claudiohbsantos commented 3 years ago

This looks awesome, thanks for all the work on it and the many revisions