-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add election editing #650
Add election editing #650
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some changes and left comments to explain them, but nothing of much importance really. The comments are just there if you're curious as to why I made the changes. Nice work! ⭐
As always, feel free to disagree 😄, otherwise we can go ahead and merge this.
"elections_content_sv": "Content (SV)", | ||
"elections_content_en": "Content (EN)", | ||
"elections_link": "Link", | ||
"elections_expiryDate": "Last application date (23:59)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to this to make it more concise, I think it should already be pretty obvious that the last application date is inclusive.
const { prisma } = locals; | ||
|
||
const committees = await prisma.committee.findMany({ | ||
orderBy: [{ shortName: "asc" }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed some whitespace because Prisma query options are already quite long as it is. Note that this is one of the rare cases where our formatter Prettier allows you to choose how something should be formatted.
|
||
export const load: PageServerLoad = async ({ locals, params }) => { | ||
const { prisma } = locals; | ||
const electionPromise = prisma.election.findFirst({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of instantly awaiting the Promise that prisma.election.findFirst
returns, I allow the second query to start before awaiting the result. This allows the queries to run at the same time.
{ | ||
...election, | ||
expiresAt: dayjs(election.expiresAt).format("YYYY-MM-DD"), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By passing an object into the superValidate
call, we can easily set default values for a form. Unfortunately it's a bit more long-winded here since we need to transform election.expiresAt
into a valid date string.
@@ -12,7 +12,7 @@ | |||
<div class="flex flex-row"> | |||
<PageHeader title={m.openElections()} /> | |||
{#if isAuthorized(apiNames.ELECTION.CREATE, data.user)} | |||
<a href={"/elections/new"} class="btn btn-primary ml-auto">+ Nytt val</a> | |||
<a href="/elections/create" class="btn btn-primary ml-auto">+ Nytt val</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to create since that's more consistent with our other pages (except for one for some reason...).
@@ -29,8 +29,9 @@ | |||
{#if isAuthorized(apiNames.ELECTION.UPDATE, data.user)} | |||
<a | |||
href={"/elections/" + election.id + "/edit"} | |||
class="btn btn-secondary btn-sm ml-auto">Redigera</a | |||
> | |||
class="btn btn-outline btn-sm ml-auto" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a lot of purple (btn-secondary
) buttons on the page, but I'm not a big fan of them so I changed this one to a more neutral color.
I'm not really an expert on UI design (clearly... 😄), but colors tend to draw attention and usually signify some kind of main action we want the user to take. Editing is more of an optional action and thus doesn't need to be highlighted.
election: Pick< | ||
Election, | ||
"markdown" | "markdownEn" | "link" | "expiresAt" | "committeeId" | ||
>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to to use TypeScript's Pick
utility type so that we can reuse the Election
type that already exists.
Adds functionality to create and edit elections.