joshghent / gifbar

🦄 Find Awesome Gif's right in your Menu Bar

MIT License

42 stars 31 forks source link

Refactor components #169

Closed felix-chin closed 3 years ago

felix-chin commented 3 years ago

Refactored to functional components and updated component extensions from .js to .jsx.

I can't add reviewers to this PR, so adding a mention here. @jamescallumyoung please review.

jamescallumyoung commented 3 years ago

Hi @felix-chin, thanks for the contribution. I'm just getting a chance to look at this and I'll review it shortly.

I have noticed that GitHub seems to be having trouble creating a diff for the changes since there are code changes and file renames. I'll push a change to revert the file renaming so the diff is more helpful. I agree that the files should have the JSX extension though, so I created another ticket #170 to rename them. Feel free to take that if you like (comment there and I'll assign you).

jamescallumyoung commented 3 years ago

@felix-chin I also noticed that Codacy is reporting a few code quality issues. I'll fix those myself since there's only a handful.

jamescallumyoung commented 3 years ago

Hey @felix-chin, the changes I made in 459d4e8 update the PR to hopefully fix the code quality issues reported by Codacy. Since we're not using TypeScript, Codacy is requiring the use of PropTypes to validate prop types in the React components now they're functional components.

Could you take a look at the fix: commit (2c391d8 to ensure the change there is correct. I believe the wrong method was being called before).

jamescallumyoung commented 3 years ago

REBASE BEFORE EDITING

@felix-chin I just rebased onto master so the changes for the file renaming are included in this branch. That will fix some of the code quality issues. I'll re-lint and fix the last issues reported by PropTypes now.

If you want to make additional changes to this PR, please run git fetch && git pull --force first, so there's no need for another merge commit.

jamescallumyoung commented 3 years ago

The quality check has passed (at long last!) so I'm happy to say LGTM 🎉

Thanks for all the work @felix-chin!

Since this is @joshghent 's repo, I'll leave this open for him to review as well. @joshghent, please merge this if you also approve of the changes here. SInce there's a lot of messy changes here, we should do a squash and merge rather than the usual rebase.

In case @felix-chin is participating in hacktoberfest, I'll add the hacktoberfest-accepted label so this will still count, even if the PR is not merged before the end of the month.

joshghent commented 3 years ago

Wow thank you both @felix-chin and @jamescallumyoung for all your hard work here - it really shines through! This is going to make the code base much cleaner and nicer to work with. I'll merge this now! Thanks again, look forward to you both contributing going forward, there is certainly lots to do! Message me on twitter @joshghent or email me josh@ghent.cloud if you'd like any assistance.

felix-chin commented 3 years ago

@jamescallumyoung and @joshghent no problem! I'm happy to help and thanks for adding the hacktoberfest-accepted label!