Refactor/show contact #95
Conversation
…Contact and ContactData interfaces in ShowContact.
… Contacts and NewContact to Interfaces. Delete prop with optional UserData since it wasn't being called anywhere.
…ns in apiCalls and move interfaces to Interfaces.tsx.
…ort in userInformation. Fix spacing.
…e companyContacts to include fetch. Fix formatting. Move interfact out of apiCalls and rename to not duplicate UserData.
…factor/ShowContact
|
The related ticket in your PR is incorrect and links to one of my issues. |
Fixed! |
src/apiCalls.tsx
Outdated
| } | ||
|
|
||
| const result = await response.json(); | ||
| const companyList = result.data.map((company: any) => ({ |
There was a problem hiding this comment.
I looked into "any" for type here. Did you try ((company: Company) ? If you already tried this and it didn't work I have some ideas about why related to how Company and CompanyAttributes are defined in Interfaces.tsx.
There was a problem hiding this comment.
Nope, this is just one I missed, so thanks for catching that!
| name: company.attributes.name, | ||
| })); | ||
| return companyList | ||
| } catch (error: any) { |
There was a problem hiding this comment.
Possible Refactor:
In your catch blocks (error: any) could alternatively be (error: unknown) for safer error handling. Your next line would need:
if (error instanceof Error) { error messages here }
There was a problem hiding this comment.
In doing some research, changing the code to error: unknown does seem like best practice, so I implemented that suggestion for all instances where error was any. Any other refactoring of error handling for uniformity or robustness could be added as a task card for refactoring.
There was a problem hiding this comment.
After implementing those changes I am going to say that this is out of the scope of this task card because when trying to make changs to the error handling cypress tests were no longer passing, so it requires more than just a quick fix and this task card was just to move interfaces and apiCalls.
src/apiCalls.tsx
Outdated
| } | ||
| } catch (error: any) { | ||
| console.error("Error adding contact:", error); |
There was a problem hiding this comment.
Does this need error.message as well?
There was a problem hiding this comment.
This can make it more readable, yes. Implemented.
… Change error type to unknown and make error testing more robust.
…d to previous error messages.
…factor/ShowContact
Type of Change
Description
This PR moves the three interfaces from ShowContact to Interfaces.
It refactors and moves the fetchShowContact method to apiCalls.
It moves the two interfaces from Contact to Interfaces.
It moves the three interfaces from NewContact to Interfaces.
It moves the two interfaces from userInformation to Interfaces.
Motivation and Context
This PR follows the single responsibility principle and moves methods to their respective files.
Added Test?
No 🙅
All previous tests still pass 🥳
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.