-
Notifications
You must be signed in to change notification settings - Fork 234
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
O3-2812: Appointments: Refactoring and Code Cleanup #965
Conversation
Size Change: -47.8 kB (-2%) Total Size: 2.81 MB
ℹ️ View Unchanged
|
Trying to do some commits do some preliminary organize the appointments ESM while not going too overboard. The majority of the hooks in the appointments ESM appear to be in various "use*.tx" files within the "hooks" directory. We might want to standardize this a bit more (it is best practice to have multiple hooks in a "use*.tx" file?) but rather making that decision now, I just try to organize things that seems clearly out of place. I refactored two resources.tx file, "appointments.resouce.ts" and 'appointments-table.ts" which basically consisted of deleting those two files and moving the only hook within them that was used into "hooks/useAppointments" There were a few other resource files with hooks that I found with the code: forms.resource.ts I started to refactor them, but held back, because they were generally well encapsulated: ie, each resource file was only used by components within the same directory as the resource file. (And this may be a better pattern than a bunch of "use*" files in a single directory anyway) I did rename "appointment-table.resource.tx" to "home-appointments.resource.tx" for clarity. Also, I didn't do a holistic check to see if all the other hooks were being used, we likely can continue to remove unused ones, perhaps as part of a larger refactor. Apologies if I've stepped on anyone's work! |
# Conflicts: # packages/esm-appointments-app/src/home-appointments/appointments-list.component.tsx
packages/esm-appointments-app/src/appointments/appointments.resources.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks, @mogoodrich.
Thanks @mogoodrich! |
Requirements
Summary
Screenshots
Related Issue
Other