LF-4672 (3) - rtk locations upgrade remove duplication#4099
LF-4672 (3) - rtk locations upgrade remove duplication#4099Duncan-Brain wants to merge 50 commits into
Conversation
Duncan-Brain
changed the title
Duncan-Brain
changed the title
| loading: false, | ||
| loaded: false, | ||
| }, | ||
| irrigationTaskReducer: { |
There was a problem hiding this comment.
Missing some reducers...
| "MAP": { | ||
| "FAIL_PATCH": "Failed to update", | ||
| "FAIL_POST": "Failed to add new", | ||
| "FAIL_DELETE": "Failed to retire", |
There was a problem hiding this comment.
Using snackbars instead of WarningBox
| @@ -0,0 +1,678 @@ | |||
| /* | |||
There was a problem hiding this comment.
This could probably be reduced as well.. specifically RadioGroup
| @@ -0,0 +1,45 @@ | |||
| import { useTranslation } from 'react-i18next'; | |||
There was a problem hiding this comment.
Not a new file the git mv did not work on this one for some reason...
| <PageTitle | ||
| title={title} | ||
| onCancel={isCreateLocationPage && onCancel} | ||
| onCancel={isCreateLocationPage ? onCancel : undefined} |
There was a problem hiding this comment.
Typings didn't like the boolean here for a function.
| ...pick(data, locationProperties), | ||
| }; | ||
| }; | ||
| const getBarnFromLocationObject = (location) => { |
There was a problem hiding this comment.
no handled by clean() and flatten() in the location api hook.
| return result; | ||
| }; | ||
|
|
||
| const propertiesToPick = { |
There was a problem hiding this comment.
These utilities are moved from the aggregation of the slices.
| const AddNewCrop = React.lazy(() => import('../containers/AddNewCrop')); | ||
| const PlantingLocation = React.lazy( | ||
| () => import('../containers/Crop/AddManagementPlan/PlantingLocation'), | ||
| const PlantingLocation = React.lazy(() => |
There was a problem hiding this comment.
lol prettier love this.. some real edits in this file.
| @@ -0,0 +1,344 @@ | |||
| /* | |||
There was a problem hiding this comment.
This story isn't the best... it is intermittently laggy and slow sometimes freezes. I think it is because the service worker is weird when working with the readiness for offlineReducer.
It is already quite broken on integration because it needs adding the store. This seems to be one of the few that isn't very pure.
So.. there room for improvement here but 🤷 ...its a story and at least it is fixed and working.
| @@ -0,0 +1,177 @@ | |||
| /* | |||
There was a problem hiding this comment.
Not explicitly all necessary to make the story work but I didn't care to test the limits of what is all needed after running into some undefined errors
|
Waiting for others to be reviewed edited before merging and fixing conflict. It doesn't affect this pr |
Duncan-Brain
changed the base branch from
rtk-upgrade/locations-rebase
to
integration
SayakaOno
left a comment
SayakaOno
left a comment
There was a problem hiding this comment.
Hi Duncan! I'm sorry for not being able to review this sooner - I finally had the chance.
It feels great to get rid of one of the biggest pieces of tech debt, thank you!
The refactor looks good overall, and I didn’t find any issues except for the Storybook-related ones. I’ve left several suggestions, but otherwise, this looks good to go!
| {allLocationTypes.map((type) => ( | ||
| <Route | ||
| key={type} | ||
| path={`/create_location/${type}`} | ||
| children={<PostLocationDetailForm locationType={type} />} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Let's move the mapping to LocationDetailsRoutes, it's cleaner.
There was a problem hiding this comment.
The two mappings don't have a common path, it is different: create_location/barn doesn't nest under barn/:location_id/details. Do you mean I should group the mapping under one so I am not mapping multiple times? I could maybe:
{allLocationTypes.map((type) => (
<>
<Route
key={type}
path={`/create_location/${type}`}
children={<PostLocationDetailForm locationType={type} />}
/>
<Route
key={type}
path={`/${type}/:location_id`}
children={<LocationDetailsRoutes locationType={type} />}
/>
</>
))}
But that would change the render order of paths and I am not sure if that is important anymore.
| <Route | ||
| key={type} | ||
| path={`/create_location/${type}`} | ||
| children={<PostLocationDetailForm locationType={type} />} |
There was a problem hiding this comment.
Can we fold this into LocationDetailsRoutes?
There was a problem hiding this comment.
I don't think so.. from the other comment they don't currently have a common path. I think that could be an improvement to make a /create path under /barn or something.
| ['organic_status']: 'Non-Organic', | ||
| ...persistedFormData, | ||
| ['transition_date']: getDateInputFormat(persistedFormData['transition_date'] || new Date()), |
There was a problem hiding this comment.
Can ['organic_status'] and ['transition_date'] be organic_status and transition_date? They look mysterious.
There was a problem hiding this comment.
This file would be a bit hard to maintain/read - let's split each component into its own file, or at least into figure types!
|
@SayakaOno Hey! I just happened to be cleaning my email and reading PR comments and saw this was a couple days ago. I will try to address your comments within this week okay! |
|
Hi @Duncan-Brain, no worries at all! Take your time, thank you!! 🙏 |
2 participants
Description
Removes ~7000 LOC ! Think of how easy it will be to make a new location now.
Removes location type specific elements for:
Adds
Review notes:
Future looking
Jira link: LF-4672
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist:
pnpm i18nto help with this)