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

feat: add disclosure for settings #451

Merged
merged 2 commits into from
Aug 12, 2024
Merged

feat: add disclosure for settings #451

merged 2 commits into from
Aug 12, 2024

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Aug 7, 2024

TL;DR

This PR introduces a folder settings modal in the folder page.

What changed?

  • Imported necessary components like Modal, Input, FormControl, etc.
  • Added folder settings modal state management using useDisclosure hook.
  • Created a new FolderSettingsModal component for folder settings.
  • Utilized the useZodForm for form handling and validation.
  • Implemented the mutation to save folder settings changes to the database.
  • Updated schema and router to support editing folder's title and permalink.

How to test?

  1. Navigate to the folder page.
  2. Click on the Folder settings button.
  3. Update the title and permalink in the modal.
  4. Click Save changes and verify if the changes are reflected.

Why make this change?

This change adds the ability to edit the folder title and permalink directly from the folder page, enhancing the user experience by providing a straightforward way to manage folder settings.


Screen.Recording.2024-08-07.at.5.47.04.PM.mov

Copy link

vercel bot commented Aug 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
isomer-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2024 4:06pm

Copy link
Contributor Author

seaerchin commented Aug 7, 2024

@seaerchin seaerchin mentioned this pull request Aug 7, 2024
@seaerchin seaerchin force-pushed the 06-folder-settings branch from ed4c094 to 3722911 Compare August 7, 2024 09:47
@seaerchin seaerchin force-pushed the 06-folder-settings branch from 3722911 to a55d173 Compare August 8, 2024 06:56
@seaerchin seaerchin force-pushed the 06-folder-settings branch from a55d173 to a9dfeb5 Compare August 8, 2024 07:16
@seaerchin seaerchin force-pushed the 06-folder-settings branch from a9dfeb5 to 832c3a8 Compare August 8, 2024 07:25
@seaerchin seaerchin force-pushed the 06-folder-settings branch from 832c3a8 to d1e8ab5 Compare August 8, 2024 07:26
@seaerchin seaerchin force-pushed the 06-folder-settings branch from d1e8ab5 to 8deb8ef Compare August 8, 2024 07:43
import { AdminCmsSidebarLayout } from "~/templates/layouts/AdminCmsSidebarLayout"
import { trpc } from "~/utils/trpc"

const folderPageSchema = z.object({
siteId: z.coerce.number(),
folderId: z.coerce.number(),
siteId: z.string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

should this continue to be int? since schema is still an int.
folderId can be string because folder's id is bigint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wa actually i changed it to string here - i agree with the reasoning you gave that the bigint is just to autoincrement automatically for id, but on the frontend we can just treat as a string since we'll never interact with it.

i think changing to string here is ok - i'll have a PR to change number -> string later on wdyt?

Comment on lines +80 to +55
siteId: parseInt(siteId),
resourceId: parseInt(folderId),
Copy link
Contributor

Choose a reason for hiding this comment

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

we should refrain from all the casting and transformations imo. easy to desync or lose reasoning on why the types are the way they are. as discussed offline, since we do not actually do any arithmetic on the resource ids, we technically can keep them in their expected types: siteId: int, resourceId: string. Server should be able to pass them to the query without any transformations too

Copy link
Contributor Author

@seaerchin seaerchin Aug 12, 2024

Choose a reason for hiding this comment

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

i agree. how do we feel about a PR later on to tidy these types since they are unlikely to change? i want to do a 1 shot change rather than in phases so that post change, it's easier to reference and take code

apps/studio/src/server/modules/folder/folder.router.ts Outdated Show resolved Hide resolved
apps/studio/src/server/modules/folder/folder.router.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm after you fix ci

Copy link
Contributor Author

seaerchin commented Aug 12, 2024

Merge activity

  • Aug 12, 11:51 AM EDT: @seaerchin started a stack merge that includes this pull request via Graphite.
  • Aug 12, 12:01 PM EDT: Graphite rebased this pull request as part of a merge.
  • Aug 12, 12:07 PM EDT: @seaerchin merged this pull request with Graphite.

@seaerchin seaerchin changed the base branch from 05-add-view to graphite-base/451 August 12, 2024 15:52
@seaerchin seaerchin changed the base branch from graphite-base/451 to main August 12, 2024 15:59
@seaerchin seaerchin merged commit 0414c28 into main Aug 12, 2024
16 of 17 checks passed
@seaerchin seaerchin deleted the 06-folder-settings branch August 12, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants