Refactor code base for better enhancement and approach#7
Refactor code base for better enhancement and approach#7Claudiohbsantos merged 37 commits intomaterial-extensions:masterfrom shivapoudel:refactor/extension
Conversation
|
@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 :) |
|
@Claudiohbsantos Okay will look at it tomorrow :) |
|
@Claudiohbsantos Now you can proceed with the review. I have managed to fix the coding rulesets you have mentioned earlier including fix for If there is anything missing, please suggest me and I will have a look!! |
Claudiohbsantos
left a comment
Claudiohbsantos
left a comment
There was a problem hiding this comment.
This is a really good PR, thanks a lot. In particular I appreciate the optimizations to the building scripts and the organization improvements.
I made a few comments with changes that I'd love to see before we merge. I also haven't had a chance to pull this PR and run all the scripts in my machine (Windows) to make sure there are no regressions, I'll try to do that before the end of the week.
Feel free to respond to comments in their threads, that might be the easiest way to keep track of them.
Don't hurry to make these changes, I don't think I'll be able to look at them for a couple of days.
Thanks!
|
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 |
|
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 😄 |
|
@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 :) |
…B also its a production build
…x is also currently selecter
This is because MS Edge requires Large promotional tile with 1400px whereas chrome requires this size for marquee promo tile. This is for edge and chrome consitency only
|
@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 |
|
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 |
Solved merge conflicts proposal
|
@Claudiohbsantos I have tested the PR raised in the fork and seems nicer. Please proceed :) |
|
@Claudiohbsantos Is downloading Chromium still necessary? |
|
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 |
|
@Claudiohbsantos While running |
|
@Claudiohbsantos Merge conflicts and other issues have been patched. Please have a review and merge the PR :) |
|
Some small issues in Ubuntu (WSL), will open a pull to fix those |
|
This looks awesome, thanks for all the work on it and the many revisions |
2 participants
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 :)