Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Add locations CRUD UI #342

Merged
merged 39 commits into from
Mar 14, 2020
Merged

Conversation

Zeko369
Copy link
Member

@Zeko369 Zeko369 commented Feb 16, 2020

Add UI for #340

@Zeko369 Zeko369 changed the title Add locations CRUD UI [WIP] Add locations CRUD UI Feb 16, 2020
@Zeko369 Zeko369 mentioned this pull request Feb 19, 2020
3 tasks
@Zeko369 Zeko369 changed the title [WIP] Add locations CRUD UI Add locations CRUD UI Feb 27, 2020
@timmyichen timmyichen self-requested a review February 27, 2020 16:31
Copy link
Contributor

@timmyichen timmyichen left a comment

Choose a reason for hiding this comment

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

Mostly looks good, a few comments.

client/components/LocationForm.tsx Outdated Show resolved Hide resolved
client/components/DashboardLocation.tsx Outdated Show resolved Hide resolved
client/store/actions/locations.ts Outdated Show resolved Hide resolved
client/store/actions/locations.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,19 @@
export const FETCH_START = 'fcc/chapter/LOCATIONS_START';
Copy link
Contributor

Choose a reason for hiding this comment

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

these probably aren't necessary with typescript, but it doesn't matter for now

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not? They're simpler to use because you can easily autocomplete them + less to type

pages/dashboard/locations/[id]/edit.tsx Outdated Show resolved Hide resolved
pages/dashboard/locations/[id]/index.tsx Outdated Show resolved Hide resolved
pages/dashboard/locations/index.tsx Outdated Show resolved Hide resolved
data,
),
);
router.replace('/dashboard/locations');
Copy link
Contributor

Choose a reason for hiding this comment

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

why replace instead of push?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, here we could have a push. But this was copied from the create action to which you don't want to go back to, unless you want to create multiple of the same which you most likely don't want to do


const onSubmit = async data => {
await dispatch(locationActions.create(data));
router.replace('/dashboard/locations');
Copy link
Contributor

Choose a reason for hiding this comment

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

same re replace vs push. push is generally preferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but here I'd argue that we should keep replace because when you create something you don't want to go back to the new page. Or do we maybe? (longer explanation in a comment before)

@Zeko369 Zeko369 requested a review from timmyichen March 14, 2020 10:22
@Zeko369 Zeko369 merged commit b992a97 into freeCodeCamp:master Mar 14, 2020
@Zeko369 Zeko369 deleted the feature/locationsCRUDUI branch March 14, 2020 16:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants