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 V2 version selector so that its logic can be reused #124

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

roluk
Copy link
Member

@roluk roluk commented Nov 26, 2024

git cannot produce a good diff of accumulated changes because I move the code and files around. I suggest going through each commit individually when doing a thorough review.

  • Simplify code by using shorter syntax and making source file tree shallower
  • Reformat code to mostly not exceed 100 columns
  • Create separation between named version loading state and comparison job tracking state. Make it easier to grasp how each function affects the component state.
  • Remove duplicated state. Since the dialog is only open when it's mounted, remove open dialog checks, and use React effect disposal for cancellation signalling.
  • Improve cohesion and coupling of useNamedVersionLoading hook so that we can reuse it with minimal impact to future code complexity

The code became easier to read, IMO. If you discover any unwanted behavior, I'll defer fixing it to another PR unless it's a regression.

@roluk roluk requested a review from a team as a code owner November 26, 2024 16:11
@roluk roluk requested a review from CalebGerman November 26, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants