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

Frontend edit role page #232

Merged
merged 9 commits into from
May 17, 2024
Merged

Frontend edit role page #232

merged 9 commits into from
May 17, 2024

Conversation

badduck32
Copy link
Contributor

Pagina is nu normaal gezien af. Voornaamste vreemde dingen zijn hoe ik de users url gebruik (lijn 44 in EditRole.tsx en lijn 41 in requests.d.ts) en de eigen gedefinieerde UsersListItem type (lijn 12 in UserList.tsx)

Als er cleanere manieren zijn om dit te doen doe ik dat graag, ik sta open voor suggesties.

Closes #192

@badduck32 badduck32 added this to the Milestone 2 milestone May 2, 2024
@badduck32 badduck32 requested a review from usserwoutV2 May 2, 2024 23:35
@badduck32 badduck32 self-assigned this May 2, 2024
@@ -39,7 +38,7 @@ export enum ApiRoutes {

TEST = "api/test",
USER = "api/users/:id",
USERS = "api/users",
USERS = "api/users?:params",
Copy link
Contributor

Choose a reason for hiding this comment

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

?:params hoeft hier niet bij

Copy link
Contributor

@usserwoutV2 usserwoutV2 May 3, 2024

Choose a reason for hiding this comment

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

Er waren waarschijnlijk merge conflicts tijdens het mergen van frontend branch. Ik zie dat er een aantal dingen weg zijn (zoals [ApiRoutes.COURSE_MEMBER]: { relation: CourseRelation } ...). Best om deze terug toe te voegen.

@@ -101,7 +110,6 @@
"options": "Aanpassen",
"groupMembers": "Groepsleden",
"newProject": "Nieuw project",
"scoreTooHigh": "Score is hoger dan maximum score",
Copy link
Contributor

Choose a reason for hiding this comment

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

Dit mag niet weg.

@@ -98,7 +107,6 @@
"options": "Update",
"groupMembers": "Group members",
"newProject": "New project",
"scoreTooHigh": "Score is higher than maximum score",
Copy link
Contributor

Choose a reason for hiding this comment

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

Zelfde hier

Copy link
Contributor

Choose a reason for hiding this comment

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

Ik heb nog een probleem: als ik eerst iets in email toevoeg en verwijder en dan naam dan krijg ik email=&name=Wout wat niks terug geeft.
Nu is er een aparte input voor name en surname. Misschien kunnen we best een issue aanmaken om dit te veranderen naar 1 veld: "name" (matcht voor- en achternaam).

form={form}
name="search"
onFinish={onSearch}
initialValues={{ remember: true }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Er is geen form element met als naam remember, dus dit mag weg.

{ type: 'email', message: t("editRole.emailError") },
]}
>
<Input placeholder={t("editRole.email")} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Je gebruikt 3 inputs, dit is oke maar werkt niet zo goed voor kleine schermen. Misschien kan je eens naar Search kijken waar een dropdown in het begin staat en misschien de search knop aan de andere kant.
Scherm­afbeelding 2024-05-03 om 11 20 22

.map(([key, value]) => `${key}=${value}`)
.join('&');
console.log(queryString);
apiCall.get(ApiRoutes.USERS,{params:queryString}).then((res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Om query strings toe te voegen aan je url kan je new URL gebruiken.
Als je dit doet zal ts ambetant doen, maar je kan het dan casten naar ApiRoutes.USERS. Je zal wel nog de "?:params" weg moeten halen in ApiRoutes enum.
Vb:

      const params = new URLSearchParams();
      params.append('email', 'value1');
      params.append('name', 'value2');
   
      apiCall.get(ApiRoutes.USERS+"?" + params.toString() as ApiRoutes.USERS).then((res) => {
        console.log(res.data)
        setUsers(res.data);
      })

@usserwoutV2 usserwoutV2 self-requested a review May 3, 2024 09:45
Copy link
Contributor

@usserwoutV2 usserwoutV2 left a comment

Choose a reason for hiding this comment

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

oops ik had op verkeerde review status geklikt

@badduck32 badduck32 requested a review from usserwoutV2 May 16, 2024 15:00
Copy link
Contributor

@usserwoutV2 usserwoutV2 left a comment

Choose a reason for hiding this comment

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

Oki top, werkt.

@usserwoutV2 usserwoutV2 merged commit 395125f into frontend May 17, 2024
1 check passed
@usserwoutV2 usserwoutV2 deleted the frontend-edit-role2 branch May 17, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants