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.

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

fix: Added a common dropdown component #5826

Merged
merged 4 commits into from
Oct 15, 2024
Merged

fix: Added a common dropdown component #5826

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a50a00e
fix: Added a common dropdown component
gakshita Oct 14, 2024
b56b835
fix: dropdown
gakshita Oct 14, 2024
9893319
fix: estimate dropdown
gakshita Oct 15, 2024
f0e1355
fix: removed consoles
gakshita Oct 15, 2024
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
33 changes: 7 additions & 26 deletions web/ce/components/cycles/analytics-sidebar/base.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@ import { FC, Fragment } from "react";
import { observer } from "mobx-react";
// plane ui
import { TCycleEstimateType } from "@plane/types";
import { EEstimateSystem } from "@plane/types/src/enums";
import { CustomSelect, Loader } from "@plane/ui";
import { Loader } from "@plane/ui";
// components
import ProgressChart from "@/components/core/sidebar/progress-chart";
import { cycleEstimateOptions, validateCycleSnapshot } from "@/components/cycles";
import { EstimateTypeDropdown, validateCycleSnapshot } from "@/components/cycles";
// helpers
import { getDate } from "@/helpers/date-time.helper";
// hooks
import { useCycle, useProjectEstimates } from "@/hooks/store";
import { useCycle } from "@/hooks/store";

type ProgressChartProps = {
workspaceSlug: string;
Expand All @@ -24,7 +23,6 @@ export const SidebarChart: FC<ProgressChartProps> = observer((props) => {
// hooks
const { getEstimateTypeByCycleId, getCycleById, fetchCycleDetails, fetchArchivedCycleDetails, setEstimateType } =
useCycle();
const { currentActiveEstimateId, areEstimateEnabledByProjectId, estimateById } = useProjectEstimates();

// derived data
const cycleDetails = validateCycleSnapshot(getCycleById(cycleId));
Expand All @@ -33,10 +31,7 @@ export const SidebarChart: FC<ProgressChartProps> = observer((props) => {
const totalEstimatePoints = cycleDetails?.total_estimate_points || 0;
const totalIssues = cycleDetails?.total_issues || 0;
const estimateType = getEstimateTypeByCycleId(cycleId);
const isCurrentProjectEstimateEnabled = Boolean(projectId && areEstimateEnabledByProjectId(projectId));
const estimateDetails =
isCurrentProjectEstimateEnabled && currentActiveEstimateId && estimateById(currentActiveEstimateId);
const isCurrentEstimateTypeIsPoints = estimateDetails && estimateDetails?.type === EEstimateSystem.POINTS;

const chartDistributionData =
estimateType === "points" ? cycleDetails?.estimate_distribution : cycleDetails?.distribution || undefined;

Expand All @@ -63,23 +58,9 @@ export const SidebarChart: FC<ProgressChartProps> = observer((props) => {
};
return (
<>
{isCurrentEstimateTypeIsPoints && (
<div className="relative flex items-center justify-between gap-2 pt-4">
<CustomSelect
value={estimateType}
label={<span>{cycleEstimateOptions.find((v) => v.value === estimateType)?.label ?? "None"}</span>}
onChange={onChange}
maxHeight="lg"
buttonClassName="border-none rounded text-sm font-medium capitalize"
>
{cycleEstimateOptions.map((item) => (
<CustomSelect.Option key={item.value} value={item.value} className="capitalize">
{item.label}
</CustomSelect.Option>
))}
</CustomSelect>
</div>
)}
<div className="relative flex items-center justify-between gap-2 pt-4">
<EstimateTypeDropdown value={estimateType} onChange={onChange} cycleId={cycleId} projectId={projectId} />
</div>
<div className="py-4">
<div>
<div className="relative flex items-center gap-2">
Expand Down
33 changes: 4 additions & 29 deletions web/core/components/cycles/active-cycle/productivity.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { FC, Fragment } from "react";
import { observer } from "mobx-react";
import Link from "next/link";
import { ICycle, TCycleEstimateType, TCyclePlotType } from "@plane/types";
import { CustomSelect, Loader } from "@plane/ui";
import { ICycle, TCycleEstimateType } from "@plane/types";
import { Loader } from "@plane/ui";
// components
import ProgressChart from "@/components/core/sidebar/progress-chart";
import { EmptyState } from "@/components/empty-state";
Expand All @@ -11,23 +11,18 @@ import { EmptyStateType } from "@/constants/empty-state";
import { useCycle, useProjectEstimates } from "@/hooks/store";
// plane web constants
import { EEstimateSystem } from "@/plane-web/constants/estimates";
import { EstimateTypeDropdown } from "../dropdowns/estimate-type-dropdown";

export type ActiveCycleProductivityProps = {
workspaceSlug: string;
projectId: string;
cycle: ICycle | null;
};

const cycleBurnDownChartOptions = [
{ value: "issues", label: "Issues" },
{ value: "points", label: "Points" },
];

export const ActiveCycleProductivity: FC<ActiveCycleProductivityProps> = observer((props) => {
const { workspaceSlug, projectId, cycle } = props;
// hooks
const { getEstimateTypeByCycleId, setEstimateType } = useCycle();
const { currentActiveEstimateId, areEstimateEnabledByProjectId, estimateById } = useProjectEstimates();

// derived values
const estimateType: TCycleEstimateType = (cycle && getEstimateTypeByCycleId(cycle.id)) || "issues";
Expand All @@ -37,11 +32,6 @@ export const ActiveCycleProductivity: FC<ActiveCycleProductivityProps> = observe
setEstimateType(cycle.id, value);
};

const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : false;
const estimateDetails =
isCurrentProjectEstimateEnabled && currentActiveEstimateId && estimateById(currentActiveEstimateId);
const isCurrentEstimateTypeIsPoints = estimateDetails && estimateDetails?.type === EEstimateSystem.POINTS;

const chartDistributionData =
cycle && estimateType === "points" ? cycle?.estimate_distribution : cycle?.distribution || undefined;
const completionChartDistributionData = chartDistributionData?.completion_chart || undefined;
Expand All @@ -52,22 +42,7 @@ export const ActiveCycleProductivity: FC<ActiveCycleProductivityProps> = observe
<Link href={`/${workspaceSlug}/projects/${projectId}/cycles/${cycle?.id}`}>
<h3 className="text-base text-custom-text-300 font-semibold">Issue burndown</h3>
</Link>
{isCurrentEstimateTypeIsPoints && (
<div className="relative flex items-center gap-2">
<CustomSelect
value={estimateType}
label={<span>{cycleBurnDownChartOptions.find((v) => v.value === estimateType)?.label ?? "None"}</span>}
onChange={onChange}
maxHeight="lg"
>
{cycleBurnDownChartOptions.map((item) => (
<CustomSelect.Option key={item.value} value={item.value}>
{item.label}
</CustomSelect.Option>
))}
</CustomSelect>
</div>
)}
<EstimateTypeDropdown value={estimateType} onChange={onChange} cycleId={cycle.id} projectId={projectId} />
</div>

<Link href={`/${workspaceSlug}/projects/${projectId}/cycles/${cycle?.id}`}>
Expand Down
39 changes: 39 additions & 0 deletions web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import React from "react";
import { TCycleEstimateType } from "@plane/types";
import { CustomSelect } from "@plane/ui";
import { useCycle, useProjectEstimates } from "@/hooks/store";
import { cycleEstimateOptions } from "../analytics-sidebar";

type TProps = {
value: TCycleEstimateType;
onChange: (value: TCycleEstimateType) => Promise<void>;
showDefault?: boolean;
projectId: string;
cycleId: string;
};

export const EstimateTypeDropdown = (props: TProps) => {
const { value, onChange, projectId, cycleId, showDefault = false } = props;
const { getIsPointsDataAvailable } = useCycle();
const { areEstimateEnabledByProjectId } = useProjectEstimates();
const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : false;
return getIsPointsDataAvailable(cycleId) || isCurrentProjectEstimateEnabled ? (
<div className="relative flex items-center gap-2">
<CustomSelect
value={value}
label={<span>{cycleEstimateOptions.find((v) => v.value === value)?.label ?? "None"}</span>}
onChange={onChange}
maxHeight="lg"
buttonClassName="bg-custom-background-90 border-none rounded text-sm font-medium "
>
{cycleEstimateOptions.map((item) => (
<CustomSelect.Option key={item.value} value={item.value}>
{item.label}
</CustomSelect.Option>
))}
</CustomSelect>
</div>
) : showDefault ? (
<span className="capitalize">{value}</span>
) : null;
};
Comment on lines +1 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding prop validation for improved robustness.

The component is well-structured and follows the Single Responsibility Principle. To further improve its robustness, consider adding prop validation.

You can use PropTypes (if you prefer runtime checking) or leverage TypeScript's built-in type checking by explicitly declaring the component's prop types:

const EstimateTypeDropdown: React.FC<TProps> = ({ 
  value, 
  onChange, 
  projectId, 
  cycleId, 
  showDefault = false 
}) => {
  // Component logic
};

This change will provide better type inference and catch potential prop-related errors at compile-time.

🧰 Tools
🪛 Biome

[error] 19-19: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

1 change: 1 addition & 0 deletions web/core/components/cycles/dropdowns/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export * from "./filters";
export * from "./estimate-type-dropdown";
15 changes: 15 additions & 0 deletions web/core/store/cycle.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { isFuture, isPast, isToday } from "date-fns";
import isEmpty from "lodash/isEmpty";
import set from "lodash/set";
import sortBy from "lodash/sortBy";
import { action, computed, observable, makeObservable, runInAction } from "mobx";
Expand Down Expand Up @@ -58,6 +59,8 @@ export interface ICycleStore {
getProjectCycleIds: (projectId: string) => string[] | null;
getPlotTypeByCycleId: (cycleId: string) => TCyclePlotType;
getEstimateTypeByCycleId: (cycleId: string) => TCycleEstimateType;
getIsPointsDataAvailable: (cycleId: string) => boolean;

// actions
updateCycleDistribution: (distributionUpdates: DistributionUpdates, cycleId: string) => void;
validateDate: (workspaceSlug: string, projectId: string, payload: CycleDateCheckData) => Promise<any>;
Expand Down Expand Up @@ -271,6 +274,18 @@ export class CycleStore implements ICycleStore {
return this.cycleMap?.[this.currentProjectActiveCycleId!] ?? null;
}

getIsPointsDataAvailable = computedFn((cycleId: string) => {
console.log(this.cycleMap[cycleId]?.version);
const cycle = this.cycleMap[cycleId];
if (!cycle) return false;
if (this.cycleMap[cycleId].version === 2) return cycle.progress.some((p) => p.total_estimate_points > 0);
else if (this.cycleMap[cycleId].version === 1) {
console.log({ ...cycle.estimate_distribution?.completion_chart });
const completionChart = cycle.estimate_distribution?.completion_chart || {};
return !isEmpty(completionChart) && Object.keys(completionChart).some((p) => completionChart[p]! > 0);
} else return false;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve getIsPointsDataAvailable method implementation.

The new method looks good overall, but there are a few improvements we can make:

  1. Remove the console.log statements (lines 278 and 283) as they shouldn't be in production code.
  2. Handle the case where the cycle version is undefined.
  3. Simplify the nested if-else structure for better readability.

Here's a suggested refactor:

 getIsPointsDataAvailable = computedFn((cycleId: string) => {
-    console.log(this.cycleMap[cycleId]?.version);
     const cycle = this.cycleMap[cycleId];
     if (!cycle) return false;
-    if (this.cycleMap[cycleId].version === 2) return cycle.progress.some((p) => p.total_estimate_points > 0);
-    else if (this.cycleMap[cycleId].version === 1) {
-      console.log({ ...cycle.estimate_distribution?.completion_chart });
+    
+    switch(cycle.version) {
+      case 2:
+        return cycle.progress.some((p) => p.total_estimate_points > 0);
+      case 1:
       const completionChart = cycle.estimate_distribution?.completion_chart || {};
       return !isEmpty(completionChart) && Object.keys(completionChart).some((p) => completionChart[p]! > 0);
-    } else return false;
+      default:
+        return false;
+    }
 });

This refactored version improves readability and handles all cases more explicitly.

📝 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
getIsPointsDataAvailable = computedFn((cycleId: string) => {
console.log(this.cycleMap[cycleId]?.version);
const cycle = this.cycleMap[cycleId];
if (!cycle) return false;
if (this.cycleMap[cycleId].version === 2) return cycle.progress.some((p) => p.total_estimate_points > 0);
else if (this.cycleMap[cycleId].version === 1) {
console.log({ ...cycle.estimate_distribution?.completion_chart });
const completionChart = cycle.estimate_distribution?.completion_chart || {};
return !isEmpty(completionChart) && Object.keys(completionChart).some((p) => completionChart[p]! > 0);
} else return false;
});
getIsPointsDataAvailable = computedFn((cycleId: string) => {
const cycle = this.cycleMap[cycleId];
if (!cycle) return false;
switch(cycle.version) {
case 2:
return cycle.progress.some((p) => p.total_estimate_points > 0);
case 1:
const completionChart = cycle.estimate_distribution?.completion_chart || {};
return !isEmpty(completionChart) && Object.keys(completionChart).some((p) => completionChart[p]! > 0);
default:
return false;
}
});


/**
* returns active cycle progress for a project
*/
Expand Down
Loading