Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: App to functional component #662

Merged
merged 2 commits into from
Aug 20, 2020
Merged

refactor: App to functional component #662

merged 2 commits into from
Aug 20, 2020

Conversation

JF-Cozy
Copy link
Contributor

@JF-Cozy JF-Cozy commented Aug 20, 2020

This PR is a part [2] of multiple PR about the new sort logic in the application. Tests will maybe failed and probably don't after merging previous PR and rebasing this one.

  • Refactor App to use functional component / hooks
  • (*) Extract the main query to new file
  • Cleanup browser index (app init) and store configuration

(*) the Query is still not using hooks. It is the very first refactor, the hook will comes in an other PR

@JF-Cozy JF-Cozy force-pushed the refactor/AppToFunc branch from be1df92 to 18e670a Compare August 20, 2020 12:49
@JF-Cozy JF-Cozy requested a review from Crash-- August 20, 2020 12:50
@Crash--
Copy link
Contributor

Crash-- commented Aug 20, 2020

Tests are red.

Also, if I had to be a pain in the ass I'd say you should have made a commit for the hook translate, one for the breakpoint etc.

@JF-Cozy JF-Cozy force-pushed the refactor/AppToFunc branch from 18e670a to 9563b28 Compare August 20, 2020 14:36
@JF-Cozy JF-Cozy force-pushed the refactor/AppToFunc branch from 9563b28 to 4b40a16 Compare August 20, 2020 14:38
@JF-Cozy
Copy link
Contributor Author

JF-Cozy commented Aug 20, 2020

Tests are red.

Yes I know, as I said in the PR description Tests will maybe failed and probably don't after merging previous PR and rebasing this one.. So now previous PR is merged, this one rebased and it's ok.

Also, if I had to be a pain in the ass I'd say you should have made a commit for the hook translate, one for the breakpoint etc.

Hmmm hooks like those just add a const and remove entries in the export. And a wrapper in the breakpoint case... so just few changes, but ok next time I will do it by hooks.

@JF-Cozy JF-Cozy force-pushed the refactor/AppToFunc branch from 4b40a16 to 44f770e Compare August 20, 2020 14:45
@JF-Cozy JF-Cozy force-pushed the refactor/AppToFunc branch from 44f770e to 4a2972a Compare August 20, 2020 14:47
@JF-Cozy JF-Cozy requested a review from Crash-- August 20, 2020 14:48
@JF-Cozy JF-Cozy added the merge label Aug 20, 2020
@Crash--
Copy link
Contributor

Crash-- commented Aug 20, 2020

Hmmm hooks like those just add a const and remove entries in the export. And a wrapper in the breakpoint case... so just few changes, but ok next time I will do it by hooks.

It's just way easier to review commit by commit with one commit dedicated to one thing.

@JF-Cozy
Copy link
Contributor Author

JF-Cozy commented Aug 20, 2020

Can be merged if ok, previous PR #661 was merged

@JF-Cozy
Copy link
Contributor Author

JF-Cozy commented Aug 20, 2020

Tests are red.

Yes I know, as I said in the PR description Tests will maybe failed and probably don't after merging previous PR and rebasing this one.. So now previous PR is merged, this one rebased and it's ok.

My mistake it's not in this PR :( I add it right now, so my apologies

@JF-Cozy JF-Cozy merged commit 71c1dbb into master Aug 20, 2020
@JF-Cozy JF-Cozy deleted the refactor/AppToFunc branch September 21, 2020 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants