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

Moving files ONLY: some reorganization #30

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Conversation

bbland1
Copy link
Collaborator

@bbland1 bbland1 commented Sep 1, 2024

For an example of how to fill this template out, see this Pull Request.

Description

With the mention of moving some files in the views of in PR #27. I added the authenticaed and unauthenticated subdirectories in views and just moved ManageList, List and PageNotFound in to their proper directories.

Related Issue

There is no current issue.

Type of Changes

Type
♻️ Refactor

Testing Steps / QA Criteria

  • There shouldn't be any functional changes, but please check to make sure it works on yalls computers too! 😅

Copy link

github-actions bot commented Sep 1, 2024

Visit the preview URL for this PR (updated for commit 4172332):

https://tcl-77-smart-shopping-list--pr30-bb-file-reorganizati-oniozmxc.web.app

(expires Sun, 08 Sep 2024 18:08:47 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: b77df24030dca7d8b6561cb24957d2273c5e9d72

@bbland1 bbland1 self-assigned this Sep 1, 2024
@bbland1 bbland1 changed the title moving our auth/unauth routes into new auth/unauth sub directories Moving files ONLY: some reorganization Sep 1, 2024
@bbland1 bbland1 marked this pull request as ready for review September 1, 2024 18:10
@bbland1
Copy link
Collaborator Author

bbland1 commented Sep 1, 2024

Continuing on a comment that @tannaurus made on PR #27 about organizing the code a bit more as the react app gets bigger. After merging the #27 I made this PR to add the authenticated and unauthenticated subdirectories within views and moved the few files that fall into those (updating imports where needed).

I wanted to pose the question if there were any additional subdirectories everyone would want to add to components it is starting to get bigger and will as we add functionality so I figured while we are doing a move only PR if any suggestions it would be a time!

I know @RossaMania and @eternalmaha made the forms subdirectory and I was thinking maybe CreateList and FilterListInput could go in there since they are all forms. Then maybe some other ones but wasn't sure so wanted to make the PR to ask 😄

@RossaMania
Copy link
Collaborator

How do we feel about an AddItemForm.tsx component? I feel like all the logic for adding items and selecting when to buy, as well as lines 86-158 in views/ManageList.tsx could be in this component.

@bbland1
Copy link
Collaborator Author

bbland1 commented Sep 4, 2024

How do we feel about an AddItemForm.tsx component? I feel like all the logic for adding items and selecting when to buy, as well as lines 86-158 in views/ManageList.tsx could be in this component.

I am not oppose to that at all, I think that logic in ManageList should be its own component.

What I am not not sure on if adding that file and moving that would fall into a reorganization. I took it as we are moving the files we have to be in better organized sub directories not moving any of the logic as of now.🤔

Copy link
Collaborator

@alex-andria alex-andria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty simple refactor! I see this is from the recommendation by Tanner in creating a cleaner directory structure for authenticated and unauthenticated directories from PR #27 like you said.

Still works functionally when testing. If the other Collabies are on the same page and approved this one should be good to go! Thanks for the initiative in getting this PR out @bbland1 🙌 🙌

@tannaurus
Copy link
Collaborator

How do we feel about an AddItemForm.tsx component? I feel like all the logic for adding items and selecting when to buy, as well as lines 86-158 in views/ManageList.tsx could be in this component.

I am not oppose to that at all, I think that logic in ManageList should be its own component.

What I am not not sure on if adding that file and moving that would fall into a reorganization. I took it as we are moving the files we have to be in better organized sub directories not moving any of the logic as of now.🤔

I like both of these ideas a lot! Ross, your proposed refactored sounds great. I agree with Brianna that we could do that in another PR if someone has the bandwidth to pick it up 😄

@alex-andria
Copy link
Collaborator

How do we feel about an AddItemForm.tsx component? I feel like all the logic for adding items and selecting when to buy, as well as lines 86-158 in views/ManageList.tsx could be in this component.

I am not oppose to that at all, I think that logic in ManageList should be its own component.

What I am not not sure on if adding that file and moving that would fall into a reorganization. I took it as we are moving the files we have to be in better organized sub directories not moving any of the logic as of now.🤔

That's a good point! For best practice, I know smaller PR changes are best. However in this case, if you were to make that change and it works functionally I don't see a problem with making that refactor here if that's what makes sense for your group and you all agree. We just have to make sure these changes are pulled before merging the upcoming PRs for this week.

@bbland1
Copy link
Collaborator Author

bbland1 commented Sep 4, 2024

I'm gonna merge this so that. There can be later PRs and the code will be the at the same place for other PRs.

@bbland1 bbland1 merged commit 47a3e94 into main Sep 4, 2024
3 checks passed
@bbland1 bbland1 deleted the bb/file-reorganization branch September 4, 2024 14:44
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.

4 participants