search

GoogleChromeLabs / tooling.report

tooling.report a quick way to determine the best build tool for your next web project, or if tooling migration is worth it, or how to adopt a tool's best practice into your existing configuration and code base.
https://tooling.report
Apache License 2.0
848 stars 49 forks source link

solves issue #130 #450

Closed deepanshu44 closed 2 years ago

deepanshu44 commented 2 years ago

added next/prev links at the bottom of the page in test pages

argyleink commented 2 years ago

Lots of great work in there! I tried to match the comps a bit more, next pass i'll adjust their size on mobile but i need to stop reviewing for now.

Desktop ![Screen Shot 2022-01-12 at 4 11 06 PM](https://user-images.githubusercontent.com/1134620/149243020-60b6303b-ede5-4d99-b114-f585d940b5b3.png)
Mobile ![Screen Shot 2022-01-12 at 4 11 16 PM](https://user-images.githubusercontent.com/1134620/149243023-f255b32a-d7d5-4185-bdfa-07a7e60beec8.png)

after the design review, i'll check out the rest more. i gave it a quick pass and didnt see any glaring issues. thanks for all your hackin on this!

deepanshu44 commented 2 years ago

@argyleink Thanks for taking time to review this. Also, one thing, I forgot to remove backup files from my worktree while doing commits. I removed them and pushed a new commit in my dev branch. I also reordered your changes to the top in another branch.

jakearchibald commented 2 years ago

@deepanshu44 this looks great to me! Can you remove the package-lock.json from this PR? Then I'll merge it.

deepanshu44 commented 2 years ago

@jakearchibald @argyleink Thanks, but I encountered a bug while running the site on Firefox today. SVGs size seems to be broken.

Broken size ![Screenshot from 2022-02-09 00-05-13](https://user-images.githubusercontent.com/62979448/153067029-5d4174fc-7565-4e0f-88d6-894e62dd3b6b.png)

I will fix it and send another push by tomorrow at most. It was my mistake to not realize that SVGs were already implemented in TestCrumbs component and all I had to do was just copy and paste. I sincerely apologize for my mistake.

deepanshu44 commented 2 years ago

@argyleink @jakearchibald Pushed some changes. Everything seems to be working correctly in firefox and chrome.

Desktop ![Screenshot from 2022-02-09 13-03-03](https://user-images.githubusercontent.com/62979448/153144124-8e502781-ac13-4c70-9038-4df834baccc0.png)
Mobile ![Screenshot from 2022-02-09 13-15-15](https://user-images.githubusercontent.com/62979448/153144721-81e173d6-53fc-4dd0-b747-b29671008b2b.png)
jakearchibald commented 2 years ago

No need to apologize at all! We missed it in review too. I really appreciate all your work on this issue. Thanks! Shipping this now…

jakearchibald commented 2 years ago

Oh, one last thing. Can you remove the change to package-lock.json from this PR? I think we got crossed-wires earlier. When I said "remove it from this PR", I didn't mean that this PR should remove package-lock.json from the project, I meant that there shouldn't be a change to package-lock.json in this PR. This PR doesn't change package.json so there's no reason for it to make changes to package-lock.json.

deepanshu44 commented 2 years ago

@jakearchibald Thanks :-) I restored the changes made by our Bot. Edit: Latest commit I made restores the date modified as well for package-lock.json.

jakearchibald commented 2 years ago

Perfect, thank you!

deepanshu44 commented 2 years ago

Thank you, @argyleink, it's because of your active feedback this PR got accepted, and it's my first PR (apart from typos ones) :-) Also thanks @jakearchibald

I will be looking onto solving other active issues here as well in the future.