sveltekit projects writable #647
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for documentcloud-frontend ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
eyeseast
requested review from
allanlasser,
DillonBergin and
duckduckgrayduck
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
Comment on lines
+84
to
+109
| <form method="post" action="/documents/projects/" use:enhance={onSubmit}> | ||
| <h2>{$_("projects.create")}</h2> | ||
| <Field title={$_("projects.fields.title")} required inline> | ||
| <Text name="title" placeholder={$_("projects.fields.title")} /> | ||
| </Field> | ||
|
|
||
| <details> | ||
| <summary>{$_("common.more")} …</summary> | ||
| <Flex direction="column"> | ||
| <Field title={$_("projects.fields.description")}> | ||
| <TextArea name="description" /> | ||
| </Field> | ||
| <Field title={$_("projects.fields.private")} inline> | ||
| <Switch name="private" /> | ||
| </Field> | ||
| <Field title={$_("projects.fields.pinned")} inline> | ||
| <Switch name="pinned" /> | ||
| </Field> | ||
| </Flex> | ||
| </details> | ||
| <Flex> | ||
| <Button mode="primary" type="submit"> | ||
| {$_("common.add")} + | ||
| </Button> | ||
| </Flex> | ||
| </form> |
There was a problem hiding this comment.
I think that this form belongs in its own modal:
- Creating a new project is a focused and intentional interaction that waits on a server response, so it creates a new UI context. This points to a modal being the correct space for this interaction.
- Creating a new project is a less common action when organizing documents. In this UI, we can assume the frequency of a user creating new projects is less than the frequency of picking existing projects.
- Use of a
<details>element speaks to the current UI feeling too "cluttered" with the full creation form, but I think this misplaces the disclosure of additional interaction. Instead of hiding individual inputs from users, we should hide the entire form until the user makes an intentional interaction to create a new project. - This project form has no opinion about the context it's rendered in, so it could appear in any kind of layout. This means we are not necessarily stacking modals inside one interface. Our current implementation of modals also supports layering, so the question is not "will this decision result in a nested modal?" but "is a modal the appropriate interaction in the context of this component?" In this case, the answer is yes. By reframing the question away from outcome and towards motivation, it allows us to think more critically about the use of a modal in the context of each component—the culmination of those decisions is a consisently behaved UI that's more coherent to users.
We also already have the EditProject component that defines this form, so I think reusing that instead of redefining it is the correct path here for ensuring consistency in creating projects going forward.
There was a problem hiding this comment.
I think you're right, but I'm going to punt this to a separate issue/PR in favor of getting this merged sooner. I'll open an issue to refactor.
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
allanlasser
reviewed
Sep 11, 2024
There was a problem hiding this comment.
Overall looks great, most feedback is either flagging things for issues/follow-up work, or lightly refactoring. No huge changes.
Wanted to confirm that adding to projects from manager views is intentionally not a part of this PR? That's the only piece of "lifecycle" that seems missing.
Co-authored-by: Allan Lasser <[email protected]>
allanlasser
approved these changes
Sep 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Full project lifecycle:
Project user management:
Documents:
Plus lots of small fixes. When this is merged, projects are done.
Closes #643, #642, #210