Kyoso-Team / kyoso

A web application that takes osu! tournaments beyonds spreadsheets.
http://kyoso.sh
GNU Affero General Public License v3.0
1 stars 1 forks source link

Improve creation and handling of forms for better DX #11

Closed L-Mario564 closed 4 months ago

L-Mario564 commented 10 months ago

Proposition to improve the DX of creating and handling forms.

Goals

Proposed solution

We'll take the tournament/[tournamentId]/manage/staff/roles page as an example. Currently, the form to create a staff role works as follows:

// src/routes/tournament/[tournamentId]/manage/staff/roles/+page.svelte
<script lang="ts">
  import ... from ...;

  async function onCreateRole(defaultValue?: { name: string }) {
    form.create<{
      name: string;
    }>({
      defaultValue,
      title: 'Create Staff Role',
      fields: ({ field }) => [
        field('Role name', 'name', 'string', {
          validation: (z) => z.max(45)
        })
      ],
      onSubmit: ...
    })
  }
</script>

A lot of stuff is handled by client-side Typescript, which leads to a ton of abstraction and bloats up the bundle size as many of these things can be handled by using more native Svelte and HTML functionality. This also leads to a difficulty in implementing anything that may be a "one off" like putting a disclaimer or warning, as that would require fiddling further with Typescript and updating the Form component that works under the hood.

This approach is quite dated, with me creating this when starting the project, but I want to rework this into something a lot more simpler. My proposition is as follows, always taking the same page as an example:

// src/routes/tournament/[tournamentId]/manage/staff/roles/+page.svelte
<script lang="ts">
  import ... from ...;
  import CreateStaffRole from '$forms/CreateStaffRole'; // "$forms" points to `src/forms`

  async function onCreateRole(defaultValue?: { name: string }) {
    form.create(CreateStaffRole, {
      defaultValue,
      onSubmit: ...
    });
  }
</script>

// src/forms/CreateStaffRole
<script lang="ts">
  import { z } from 'zod';
  import { Form, Text } from '$components/form';
  import type { FormValue } from '$types';

  const schemas = {
    name: z.string().max(45)
  };

  let value: FormValue<typeof schemas> = {};
</script>

<Form {value}>
  <svelte:fragment slot="header">
    <h2>Create Staff Role</h2>
  </svelte:fragment>
  <Text label="Role name" name="name" schema={schemas.name} bind:value={value.name} />
</Form>

This implementation looks more verbose at first glance, but unlike the current implementation, there's not much to hide here, with the form's elements being a lot more explicit, making use of Svelte components instead of purely Typescript functions and objects. This also means that, for the sake of an example, if we want to add a description for the form, we could just add a p tag below h2 instead of having to first add the description property to the current form's store and then have it displayed in the global Form component while trying not to break the type system or anything else related to the form.

Entropy-10 commented 10 months ago

Yeah, I definitely think this is new implementation looks a lot better to work with. I don't really think there is much that I would personally input on with this new implementation, as it seems easy enough to work with and provides plenty of room for customization if needed.

L-Mario564 commented 10 months ago

@Entropy-10 What do you think about having the form be in a separate file? In this case it may seem unnecessary because it's only one field, but I also considered forms with multiple fields (like with creating a prize), separating the form's logic from the page's logic. I think that improves separation of concerns, but at the same time, you have to jump between the two files if you want to work on both of them at once.

Entropy-10 commented 10 months ago

Hmmmm, I prefer having the forms in a separate file. So then it's easy to find change any forms, because they are all in one place. As for the jumping between two files I don't really think it will be really an issue since most the work you'd be doing outside the forms file would be writing the onSumbit handler. I feel like because of the separation of concerns it actually will make working on forms quite linear. Starting with building out the schemas and UI and then moving to the route page to call the creation of the form and handle submission. So there probably won't be too much back and forth between the files.

AkinariHex commented 10 months ago

As @Entropy-10 wrote, it's not an issue to jump across multiple files, I think it's better to have forms in separate files to make them work as intended and we can simply call them from the specific file. In that case we can had everything we want in the style of the form without messing up with other forms that are not like "Roles Form" (example).