-
Notifications
You must be signed in to change notification settings - Fork 1
chore: #26 Refactor React Router example usage #27
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
Conversation
fb6fb50 to
aa7fa9b
Compare
aa7fa9b to
50bfdaa
Compare
| "dayjs": "^1.11.13", | ||
| "just-debounce-it": "^3.2.0", | ||
| "qs": "^6.14.0", | ||
| "query-string": "9.1.1", |
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.
note: small scout to bump React and TypeScript to the their most recent minor versions. A React 19 upgrade for this template is not in scope for this work.
| @@ -1,9 +1,9 @@ | |||
| import { LoginModule } from '..' | |||
| import { LoginPage } from '..' | |||
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.
note: You'll see many changes like this where I've updated the name of the exported component to align with that component's updated folder name.
| } | ||
|
|
||
| export const LoginModule: FC = () => { | ||
| export const LoginPage = () => { |
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.
note: FC is generally not needed, so I've removed it where I've made other changes.
note: As noted in the test file, you'll see lots of renaming of the components I've moved so they're component name matches their folder name.
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.
note: This was originally components/contacts/edit. The design of this page is weird as it combines both a "detail" page AND a series of edit/update forms 🤷 This is not aligned in anyway with the design system, but it's beyond the scope of this work to change it here.
The only changes I've made to this file, like most of the other page components, is replacing the use of the old "routes" constants with the new URL builders.
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.
note: This used to be components/contacts/page. It's now solely responsible for handling the redirect to the detail page from (I assume) the Agency Cloud desktop application.
| } | ||
|
|
||
| describe('PrivateRouteWrapper', () => { | ||
| it('should match a snapshot', () => { |
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.
note: PrivateRouteWrapper no longer accepts children as it renders an Outlet instead.
| <MemoryRouter> | ||
| <Routes> | ||
| <Route path="/" element={children} /> | ||
| <Route path="/foo" element="Foo" /> |
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.
note: Extra route for one of the tests so we don't need to assert on a function being called, but can instead assert on DOM content
|
|
||
| if (connectInternalRedirect && location?.pathname !== connectInternalRedirect) { | ||
| redirect(connectInternalRedirect) | ||
| return <Navigate to={connectInternalRedirect} replace /> |
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.
note: redirect is not intended for use in components; that's what Navigate is for.
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.
note: new top-level route config. Maintains lazy loading of all routes except the login page.
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.
note: This file enables all the contact pages to be lazy loaded by RR6's router. When we upgrade to RR7, these "route prop" objects will move to individual files. For now though, this is a simple way of enabling lazy loading without having to jump through too many hoops in our route config (routes.tsx)
There's one of these for the "new page" as well.
|
@ksolanki7 Apologies for the size of this PR 😬 It was going to take too long to stage it all nicely. For the most part, I don't think there's a whole lot here that needs your review, but I've added comments throughout. |
ksolanki7
left a comment
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.
@kurtdoherty Changes look good to me,
- I have run this locally, there are no issues found so far.
- I'll approve the PR now and then play with this new architecture for building routes over ABX project.
Note: I'll approach you if have any concerns or suggestion while working on the project.
fixes: #26
Scope
note: I've also smuggled in an upgrade to React 19
Out-of-scope
The following changes have been explicitly de-scoped for now:
Changes
The main changes to the folder structure are as follows:
components/contacts/listcomponents/contacts/ContactListPagecomponents/contacts/editcomponents/contacts/ContactDetailPagecomponents/contacts/formcomponents/contacts/common/ContactFormcomponents/contacts/newcomponents/contacts/CreateContactPagecomponents/contacts/pagecomponents/contacts/ViewContactDesktopRedirectcomponents/contacts/viewcomponents/contacts/ContactDetailPage/FullContactDetailscomponents/contacts/routes.tscomponents/logincomponents/LoginPagecomponents/login/routes.tscomponents/new/pagecomponents/new/NewPagecomponents/new/routes.tsconstants/routes.tscore/router.tsxroutes.tsx