Skip to content
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

[#45] Destruction list detail view #54

Merged
merged 19 commits into from
Jun 7, 2024

Conversation

SilviaAmAm
Copy link
Collaborator

@SilviaAmAm SilviaAmAm commented May 23, 2024

Fixes #45

@SilviaAmAm SilviaAmAm marked this pull request as draft May 23, 2024 15:29
@SilviaAmAm SilviaAmAm force-pushed the feature/45-destruction-list-detail-view branch 3 times, most recently from fa86858 to b19e3f0 Compare May 30, 2024 08:52
completed: "Completed",
};

function formatUser(user: User) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The landing page doesn't do this either right now, but I think we should think about combining some of the things here like the STATUS_MAPPING and the formatUser() since the landing page also uses them (and possibly others later on). For example a function formatUser() that takes in some options to customize the output would be nice, and a central place to get the status mappings from.
I think we can ignore it for now though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved the status mappings to a constants.ts files and the format user function to a utils file. Then it can be more easily reused by the landing page. Then we can expand the options once we have a better idea of what we need!


export const destructionListDetailLoader = loginRequired(
async ({ request, params }: ActionFunctionArgs) => {
if (typeof params.id === "undefined") return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure, what happens if we hit this return?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The UI crashes, because the loader didnt return anything 😄 I changed it to make it return an empty dict, and then the DestructionListDetailPage renders a (temporary) 404 page.

<DataGrid
title="Zaakdossiers"
count={allZaken.count}
objectList={allZaken.results as unknown as AttributeData[]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this type assertion (and similars in the render) needed? Don't we already type assert it higher up the tree at line 30?
const { zaken: allZaken, zaaktypeChoices } = useLoaderData() as DestructionListDetailContext;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In DestructionListDetailContext they are defined as PaginatedZaken and allZaken.results is then Zaak[]. I added it to make it stop complaining about it :S

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm yeah, the DataGrid component expects AttributeData[] array I guess, a bit sad that these things aren't compatible, should be ok then haha

@SilviaAmAm SilviaAmAm requested a review from Xaohs May 31, 2024 10:15
@SilviaAmAm SilviaAmAm force-pushed the feature/45-destruction-list-detail-view branch from 20040c3 to 8d983e8 Compare June 4, 2024 10:18
@SilviaAmAm SilviaAmAm marked this pull request as ready for review June 4, 2024 10:19
@SilviaAmAm SilviaAmAm mentioned this pull request Jun 4, 2024
@SilviaAmAm SilviaAmAm force-pushed the feature/45-destruction-list-detail-view branch from 8d983e8 to 433eca1 Compare June 4, 2024 12:21
@SilviaAmAm SilviaAmAm force-pushed the feature/45-destruction-list-detail-view branch from 433eca1 to 02b30b1 Compare June 6, 2024 08:13
@SilviaAmAm SilviaAmAm force-pushed the feature/45-destruction-list-detail-view branch from 02b30b1 to 12da0fa Compare June 7, 2024 09:17
@SilviaAmAm SilviaAmAm force-pushed the feature/45-destruction-list-detail-view branch from 12da0fa to ef39a58 Compare June 7, 2024 14:01
@SilviaAmAm SilviaAmAm merged commit f6437d7 into main Jun 7, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update destruction list page (Record manager) [backend/frontend]
3 participants