Settings and edit bio #356
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Steindt
requested review from
a team and
alfredgrip
and removed request for
a team
| <div | ||
| class={languageTag() == "sv" | ||
| ? "flex flex-col items-center md:grid md:grid-cols-2" | ||
| : "flex flex-col items-center xl:grid xl:grid-cols-2"} |
There was a problem hiding this comment.
Is this really the best way to do this? Feels kinda sketchy
There was a problem hiding this comment.
It's pretty sketchy, the english names are usually longer than the swedish ones but it either looks pretty empty or cramped if you only have 1 of them. Could otherwise decide on a longer settings page by not having notifications beside tags, but there will be a bit more scrolling if that's ok.
There was a problem hiding this comment.
Fair enough. It looks fine when I look at it, so it's not outright wrong.
There was a problem hiding this comment.
I think we should find some solution with auto columns, min-content or something like that. We never know what the tags will have as names in the future.
|
Just pushed some changes, thought it was easier than doing a bunch of code reviewing. Let me know if you agree with my changes or not! |
alfredgrip
force-pushed
the
settings-and-edit-bio
branch
from
f7f7fe6
to
13397ea
Compare
| authorize(apiNames.MEMBER.UPDATE, user); | ||
| if (!user || !user.memberId) throw redirect(302, "/home"); |
There was a problem hiding this comment.
Why? I think it's weird to redirect someone to home screen instead of just showing error. It can make the page seem confusing
There was a problem hiding this comment.
You've also removed the verification that a user can actually edit this person's bio.
There was a problem hiding this comment.
But I see, the old system seems to not check if params.studentId === user.studentId, in which case you should be able to edit. Otherwise you cannot edit your own bio.
| authorize(apiNames.MEMBER.UPDATE, user); | ||
|
|
||
| const subscriptionSettings = user | ||
| ? await prisma.subscriptionSetting.findMany({ | ||
| where: { | ||
| memberId: user.memberId, | ||
| }, | ||
| }) | ||
| : []; | ||
| if (user?.memberId == null) throw redirect(302, "/home"); | ||
|
|
||
| const subscriptionSettings = await prisma.subscriptionSetting.findMany({ | ||
| where: { | ||
| memberId: user.memberId, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
If a user is not logged in, instead of redirecting them we should show an error to be clearer. This redirect doesn't make much sense. throw error(401, "Not logged in") instead (can't remember exact syntax, check other page.server.ts files or even the authorize function
| const { user, prisma } = locals; | ||
| const form = await request.formData(); | ||
| if (!user) return fail(401, { form }); |
There was a problem hiding this comment.
this should probably use zod and superforms to handle type validation, like we do in other form inputs
| const push: string[] = []; | ||
| for (const v of form.getAll("push")) { | ||
| if (typeof v !== "string") return fail(400, { form }); | ||
| push.push(v); | ||
| } | ||
| const subscription: string[] = []; | ||
| for (const v of form.getAll("subscription")) { | ||
| if (typeof v !== "string") return fail(400, { form }); | ||
| subscription.push(v); | ||
| } | ||
| const tags: string[] = []; | ||
| for (const v of form.getAll("tag")) { | ||
| if (typeof v !== "string") return fail(400, { form }); | ||
| tags.push(v); |
There was a problem hiding this comment.
could be replaced with three one-liners like this:
const push: string[] = form.getAll("push").filter((v) => typeof v === "string");
If we want the form to fail for invalid types, I think zod should help with that. Then we don't have to do the hack above and could just do const pus = form.data.push;
| } | ||
|
|
||
| const memberId = user.memberId; | ||
| if (memberId == null) return fail(400, { form }); |
There was a problem hiding this comment.
| if (memberId == null) return fail(400, { form }); | |
| if (memberId == null) return fail(401, { form }); |
There was a problem hiding this comment.
400 is Bad Request, ie something wrong with the form data. 401 is Unauthorized, ie not logged in.
| await prisma.subscriptionSetting.deleteMany({ | ||
| where: { | ||
| memberId: memberId, | ||
| }, | ||
| }); | ||
| await prisma.subscriptionSetting.createMany({ | ||
| data: subscription.map((sub) => { | ||
| return { | ||
| memberId: memberId, | ||
| type: sub, | ||
| pushNotification: push.find((tag) => sub == tag) ? true : false, | ||
| }; | ||
| }), | ||
| }); | ||
| await prisma.member.update({ | ||
| where: { | ||
| id: memberId, | ||
| }, | ||
| data: { | ||
| subscribedTags: { | ||
| set: tags.map((tag) => { | ||
| return { | ||
| id: tag, | ||
| }; | ||
| }), | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Should probably be done in a transaction in-case one of these fails. Because in that case a user might lose all their subscription settings but none would be added or something. I know this isn't written in this PR but always good to improve our code.
Also, we don't have to remove all of the user's settings, only remove the ones not in the form data.
| @@ -96,7 +96,7 @@ | |||
| <SubscriptionTags {tags} subscribedTags={subscribedTags.subscribedTags} /> | |||
| </div> | |||
| <button | |||
| class="btn absolute bottom-0 mb-4 mt-4 w-full max-w-xl bg-primary" | |||
| class="btn absolute bottom-0 mb-4 mt-4 w-full max-w-xl bg-primary text-white" | |||
There was a problem hiding this comment.
Across the page we have white text color for buttons with primary background. I do, however, think we should btn-primary instead of bg-primary to have consequential styling across the page.
| <div | ||
| class={languageTag() == "sv" | ||
| ? "flex flex-col items-center md:grid md:grid-cols-2" | ||
| : "flex flex-col items-center xl:grid xl:grid-cols-2"} |
There was a problem hiding this comment.
I think we should find some solution with auto columns, min-content or something like that. We never know what the tags will have as names in the future.
Fixes some issues with overlapping elements in the settings page on smaller screens, fixed access for settings page and edit-bio.
Screenshot of overlapping elements:
