Skip to content

[WEB-2677] chore: draft issue issue type #5836

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.

Sign up for GitHub

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

Merged
SatishGandham merged 1 commit into from
Oct 15, 2024
+15 −9
Merged

[WEB-2677] chore: draft issue issue type #5836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions web/ce/components/issues/issue-modal/additional-properties.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ type TIssueAdditionalPropertiesProps = {
issueTypeId: string | null;
projectId: string;
workspaceSlug: string;
isDraft?: boolean;
};

export const IssueAdditionalProperties: React.FC<TIssueAdditionalPropertiesProps> = () => <></>;
2 changes: 2 additions & 0 deletions web/core/components/issues/issue-modal/base.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ export const CreateUpdateIssueModalBase: React.FC<IssuesModalProps> = observer((
issueTypeId: response.type_id,
projectId: response.project_id,
workspaceSlug: workspaceSlug.toString(),
isDraft: isDraft,
});
}

Expand Down Expand Up @@ -257,6 +258,7 @@ export const CreateUpdateIssueModalBase: React.FC<IssuesModalProps> = observer((
issueTypeId: payload.type_id,
projectId: payload.project_id,
workspaceSlug: workspaceSlug.toString(),
isDraft: isDraft,
});

setToast({
Expand Down
1 change: 1 addition & 0 deletions web/core/components/issues/issue-modal/context/issue-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export type TCreateUpdatePropertyValuesProps = {
projectId: string;
workspaceSlug: string;
issueTypeId: string | null | undefined;
isDraft?: boolean;
};

export type TIssueModalContext = {
Expand Down
1 change: 1 addition & 0 deletions web/core/components/issues/issue-modal/draft-issue-layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ export const DraftIssueLayout: React.FC<DraftIssueProps> = observer((props) => {
issueTypeId: response.type_id,
projectId,
workspaceSlug: workspaceSlug?.toString(),
isDraft: true,
});
}
};
Expand Down
19 changes: 10 additions & 9 deletions web/core/components/issues/issue-modal/form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,12 @@ export const IssueFormRoot: FC<IssueFormProps> = observer((props) => {
const submitData = !data?.id
? formData
: {
...getChangedIssuefields(formData, dirtyFields as { [key: string]: boolean | undefined }),
project_id: getValues<"project_id">("project_id"),
id: data.id,
description_html: formData.description_html ?? "<p></p>",
type_id: getValues<"type_id">("type_id"),
};
...getChangedIssuefields(formData, dirtyFields as { [key: string]: boolean | undefined }),
project_id: getValues<"project_id">("project_id"),
id: data.id,
description_html: formData.description_html ?? "<p></p>",
type_id: getValues<"type_id">("type_id"),
};
Comment on lines +198 to +203
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Ensure consistent inclusion of is_draft in submitData

In the handleFormSubmit function, the submitData object may not always include the is_draft property, which could lead to inconsistencies when submitting draft issues. While you conditionally add is_draft later, it's clearer and safer to include it explicitly during the submitData construction.

Consider modifying the submitData to always include the is_draft property:

const submitData = !data?.id
  ? formData
  : {
      ...getChangedIssuefields(formData, dirtyFields as { [key: string]: boolean | undefined }),
      project_id: getValues<"project_id">("project_id"),
      id: data.id,
      description_html: formData.description_html ?? "<p></p>",
      type_id: getValues<"type_id">("type_id"),
+     is_draft: formData.is_draft,
    };

This ensures that the draft status is consistently handled during updates.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
...getChangedIssuefields(formData, dirtyFields as { [key: string]: boolean | undefined }),
project_id: getValues<"project_id">("project_id"),
id: data.id,
description_html: formData.description_html ?? "<p></p>",
type_id: getValues<"type_id">("type_id"),
};
...getChangedIssuefields(formData, dirtyFields as { [key: string]: boolean | undefined }),
project_id: getValues<"project_id">("project_id"),
id: data.id,
description_html: formData.description_html ?? "<p></p>",
type_id: getValues<"type_id">("type_id"),
is_draft: formData.is_draft,
};


// this condition helps to move the issues from draft to project issues
if (formData.hasOwnProperty("is_draft")) submitData.is_draft = formData.is_draft;
Expand Down Expand Up @@ -323,7 +323,7 @@ export const IssueFormRoot: FC<IssueFormProps> = observer((props) => {
className={cn(
"pb-4 space-y-3",
activeAdditionalPropertiesLength > 4 &&
"max-h-[45vh] overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"
"max-h-[45vh] overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid hardcoded height values for better responsiveness

Using fixed heights like max-h-[45vh] can cause layout issues on different screen sizes. This might limit the component's flexibility and affect user experience on various devices.

Consider using dynamic sizing or responsive design utilities to enhance adaptability:

- "max-h-[45vh] overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"
+ "overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"

This change allows the container to adjust its height based on content and screen size.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"max-h-[45vh] overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"
"overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"

)}
>
<div className="px-5">
Expand Down Expand Up @@ -352,7 +352,7 @@ export const IssueFormRoot: FC<IssueFormProps> = observer((props) => {
className={cn(
"px-5",
activeAdditionalPropertiesLength <= 4 &&
"max-h-[25vh] overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"
"max-h-[25vh] overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consistent handling of container overflow

Similar to the previous comment, using max-h-[25vh] may lead to inconsistent behavior across devices. Removing fixed heights promotes a more fluid layout.

Update the className to allow content-driven sizing:

- "max-h-[25vh] overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"
+ "overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"max-h-[25vh] overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"
"overflow-hidden overflow-y-auto vertical-scrollbar scrollbar-sm"

)}
>
{projectId && (
Expand All @@ -361,6 +361,7 @@ export const IssueFormRoot: FC<IssueFormProps> = observer((props) => {
issueTypeId={watch("type_id")}
projectId={projectId}
workspaceSlug={workspaceSlug?.toString()}
isDraft={isDraft}
/>
)}
</div>
Expand Down Expand Up @@ -393,7 +394,7 @@ export const IssueFormRoot: FC<IssueFormProps> = observer((props) => {
tabIndex={getIndex("create_more")}
role="button"
>
<ToggleSwitch value={isCreateMoreToggleEnabled} onChange={() => { }} size="sm" />
<ToggleSwitch value={isCreateMoreToggleEnabled} onChange={() => {}} size="sm" />
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Handle toggle state within ToggleSwitch component

Currently, the ToggleSwitch has an empty onChange handler, and the state change is managed by the parent <div>. This approach can lead to unexpected behavior and complicates the component interaction.

It's better practice to handle the toggle state within the onChange prop of the ToggleSwitch component. Update the code as follows:

<div className="inline-flex items-center gap-1.5 cursor-pointer">
- <ToggleSwitch value={isCreateMoreToggleEnabled} onChange={() => {}} size="sm" />
+ <ToggleSwitch
+   value={isCreateMoreToggleEnabled}
+   onChange={() => onCreateMoreToggleChange(!isCreateMoreToggleEnabled)}
+   size="sm"
+ />
  <span className="text-xs">Create more</span>
</div>

Additionally, you can remove the onClick, onKeyDown, tabIndex, and role attributes from the parent <div> since the interaction is now handled within ToggleSwitch. This improves accessibility and aligns with React best practices.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<ToggleSwitch value={isCreateMoreToggleEnabled} onChange={() => {}} size="sm" />
<ToggleSwitch
value={isCreateMoreToggleEnabled}
onChange={() => onCreateMoreToggleChange(!isCreateMoreToggleEnabled)}
size="sm"
/>

<span className="text-xs">Create more</span>
</div>
)}
Expand Down