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

fix: Added a common dropdown component #5826

Merged
merged 4 commits into from
Oct 15, 2024
Merged

Conversation

gakshita
Copy link
Collaborator

@gakshita gakshita commented Oct 14, 2024

Summary

  • Added common component for estimate type dropdown

Summary by CodeRabbit

  • New Features

    • Replaced the CustomSelect component with the EstimateTypeDropdown for streamlined estimate type selection.
    • Introduced a new EstimateTypeDropdown component for selecting cycle estimates based on project settings.
  • Bug Fixes

    • Improved clarity in logic for determining the current estimate type.
  • Documentation

    • Updated types and props for new components to enhance clarity and usability.

Copy link
Contributor

coderabbitai bot commented Oct 14, 2024

Walkthrough

The changes involve significant updates to the SidebarChart and ActiveCycleProductivity components, replacing the CustomSelect component with the new EstimateTypeDropdown component for selecting estimate types. The estimate-type-dropdown.tsx file has been introduced to facilitate the dropdown functionality for cycle estimates. The overall structure and control flow of the components remain consistent, with adjustments made to enhance clarity and streamline the estimate selection process.

Changes

File Path Change Summary
web/ce/components/cycles/analytics-sidebar/base.tsx Removed CustomSelect, added EstimateTypeDropdown, streamlined estimate selection process.
web/core/components/cycles/active-cycle/productivity.tsx Removed TCyclePlotType and cycleBurnDownChartOptions, replaced CustomSelect with EstimateTypeDropdown, updated estimate type logic.
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx Introduced new component for dropdown functionality, defined types for props.
web/core/components/cycles/dropdowns/index.ts Added export for EstimateTypeDropdown to the index file.
web/core/store/cycle.store.ts Added getIsPointsDataAvailable method to check points data availability for cycles.

Possibly related PRs

  • [WEB-2007] fix: cycles loading optimization #5207: The changes in the ActiveCycleProductivity component regarding the removal of the CustomSelect component and the introduction of the EstimateTypeDropdown are directly related to the modifications in the base.tsx file of the main PR, which also involves the EstimateTypeDropdown.
  • [WEB-1989] chore: archived modules and cycles #5212: The removal of the CustomSelect component and the introduction of the EstimateTypeDropdown in both the base.tsx and productivity.tsx files indicate a direct connection, as both PRs involve similar changes to the estimate selection process.
  • Fix: Cycle graphs refactor #5745: The changes in the CycleAnalyticsProgress component, particularly the handling of cycle estimates and the removal of the CustomSelect, are directly related to the main PR's updates to the SidebarChart component, which also involves similar modifications to estimate handling.
  • [WEB-2380] chore: cycle sidebar refactor #5759: The refactoring of the cycle sidebar code, including updates to the CycleAnalyticsProgress component, aligns with the main PR's focus on enhancing the sidebar's functionality and organization, particularly in relation to cycle estimates and chart rendering.

Suggested labels

🌐frontend, 🎨UI / UX, 🌟improvement, 🧹chore

Suggested reviewers

  • SatishGandham
  • sriramveeraghanta

Poem

In the sidebar where estimates play,
A new dropdown hops in, brightening the day.
No more clutter, just options to see,
Selecting with ease, oh what glee!
With cycles aligned, let progress unfold,
In the world of data, new stories are told! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9893319 and f0e1355.

📒 Files selected for processing (1)
  • web/core/store/cycle.store.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • web/core/store/cycle.store.ts

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (3)
web/core/components/cycles/active-cycle/productivity.tsx (3)

39-39: Improve variable naming for clarity.

The new variable improves code readability. However, the variable name isCurrentEstimateTypeIsPoints is grammatically incorrect.

Consider renaming it to isCurrentEstimateTypePoints:

-  const isCurrentEstimateTypeIsPoints = Boolean(estimateDetails && estimateDetails?.type === EEstimateSystem.POINTS);
+  const isCurrentEstimateTypePoints = Boolean(estimateDetails && estimateDetails?.type === EEstimateSystem.POINTS);

51-56: LGTM: New dropdown component implemented correctly.

The CycleEstimateOptions component has been successfully integrated, replacing the previous CustomSelect. This aligns with the PR objective of adding a common dropdown component.

Consider updating the prop name to match the suggested variable name change:

 <CycleEstimateOptions
-  showEstimateSelection={isCurrentEstimateTypeIsPoints}
+  showEstimateSelection={isCurrentEstimateTypePoints}
   estimateType={estimateType}
   handleEstimateChange={onChange}
   projectId={projectId}
 />

Line range hint 1-124: LGTM: Overall changes maintain functionality while introducing new dropdown.

The changes successfully integrate the new CycleEstimateOptions component while maintaining the existing functionality for both points and issues estimates. This is consistent with the PR objective.

For consistency, consider updating all occurrences of isCurrentEstimateTypeIsPoints to isCurrentEstimateTypePoints throughout the component.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ee823d2 and a50a00e.

📒 Files selected for processing (3)
  • web/ce/components/cycles/analytics-sidebar/base.tsx (2 hunks)
  • web/core/components/cycles/active-cycle/productivity.tsx (4 hunks)
  • web/core/components/cycles/dropdowns/estimate-dropdown.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
web/core/components/cycles/dropdowns/estimate-dropdown.tsx

[error] 42-42: 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)

🔇 Additional comments (7)
web/ce/components/cycles/analytics-sidebar/base.tsx (3)

69-74: LGTM! Verify the new component's behavior.

The replacement of CustomSelect with CycleEstimateOptions aligns with the PR objective of introducing a common dropdown component for estimate types. The new component seems to encapsulate more functionality, potentially improving code reusability and maintainability.

To ensure the new component behaves as expected, please manually verify:

  1. The dropdown displays the correct estimate options.
  2. Selecting an option triggers the onChange function correctly.
  3. The UI updates appropriately when an estimate type is selected.

Additionally, run the following script to check for any remaining references to the old component:

#!/bin/bash
# Description: Check for any remaining references to CustomSelect in the project

echo "Checking for any remaining CustomSelect references:"
rg --type typescript 'CustomSelect'

Line range hint 1-108: Overall changes look good. Verify component behavior.

The changes to the SidebarChart component are focused on improving the estimate type selection UI and logic, which aligns with the PR objective of introducing a common dropdown component. The core functionality of the component, including progress chart rendering and data handling, remains intact.

To ensure the overall component behavior remains correct:

  1. Manually test the component in different scenarios (e.g., with and without estimates enabled, with different estimate types).
  2. Verify that the progress chart still renders correctly with the new estimate type selection.
  3. Run the following script to check for any unexpected changes in the component's logic:
#!/bin/bash
# Description: Check for any unexpected changes in the SidebarChart component

echo "Checking for changes in core logic of SidebarChart:"
ast-grep --lang typescript --pattern $'export const SidebarChart: FC<ProgressChartProps> = observer((props) => {
  $$$
  // handlers
  const onChange = async (value: TCycleEstimateType) => {
    $$$
  };
  $$$
})'

7-7: LGTM! Verify the new component integration.

The import changes reflect the shift from CustomSelect to CycleEstimateOptions, which aligns with the PR objective of introducing a common dropdown component. The removal of cycleEstimateOptions import suggests that the options are now handled within the new component.

To ensure the new component is correctly integrated, run the following script:

Also applies to: 10-10, 12-12

web/core/components/cycles/active-cycle/productivity.tsx (1)

4-5: LGTM: Import changes align with new functionality.

The new imports (TCycleEstimateType, Loader, and CycleEstimateOptions) are consistent with the changes in the component and the PR objective of adding a common dropdown component.

Also applies to: 14-14

web/core/components/cycles/dropdowns/estimate-dropdown.tsx (3)

13-19: TProps type is well-defined

The TProps type accurately specifies the props required by the CycleEstimateOptions component, enhancing clarity and type safety.


21-37: Dropdown component is implemented correctly

The Dropdown component effectively renders the CustomSelect component, properly managing value, label, options, and handling onChange events.


38-48: CycleEstimateOptions component properly handles conditional rendering

The CycleEstimateOptions component correctly determines whether to display the Dropdown component or the default value based on the provided props and project estimates status.

🧰 Tools
🪛 Biome

[error] 42-42: 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)

export type TDropdownProps = {
value: string;
onChange: (value: TCycleEstimateType) => Promise<void>;
options: any[];
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

Strongly type the 'options' prop in TDropdownProps

Currently, the options prop is typed as any[], which can lead to type safety issues. To enhance type safety and maintainability, consider defining a specific type for the options array.

Define an interface for the option type and update the options prop:

interface Option {
  value: string;
  label: string;
}

export type TDropdownProps = {
  value: string;
  onChange: (value: TCycleEstimateType) => Promise<void>;
  options: Option[];
};

const { showEstimateSelection, estimateType, handleEstimateChange, projectId, defaultValue = null } = props;

const { areEstimateEnabledByProjectId } = useProjectEstimates();
const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : 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

Simplify boolean assignment by removing unnecessary ternary operator

The use of the ternary operator ? true : false is unnecessary since the expression already results in a boolean value. Simplifying the assignment improves code readability.

Apply this diff to simplify the code:

-  const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : false;
+  const isCurrentProjectEstimateEnabled = Boolean(projectId && areEstimateEnabledByProjectId(projectId));
📝 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
const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : false;
const isCurrentProjectEstimateEnabled = Boolean(projectId && areEstimateEnabledByProjectId(projectId));
🧰 Tools
🪛 Biome

[error] 42-42: 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)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (4)

1-13: LGTM! Consider using a union type for value.

The imports and type definitions look good. The TProps type is well-defined and includes all necessary props.

For improved type safety, consider defining value as a union type of specific estimate types instead of using TCycleEstimateType. This would ensure that only valid estimate types can be passed to the component.

type TEstimateType = "none" | "fibonacci" | "t-shirt" | /* other valid types */;

type TProps = {
  // ... other props
  value: TEstimateType;
  // ... remaining props
};

15-19: LGTM! Simplify the boolean expression.

The component definition and hook usage look good. The destructuring of props and use of the useProjectEstimates hook are appropriate.

Simplify the boolean expression on line 19 to improve readability:

const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId);

This change removes the unnecessary ternary operator, making the code more concise and easier to understand.

🧰 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)


20-38: LGTM! Consider adding an aria-label for accessibility.

The conditional rendering logic and use of CustomSelect are well-implemented. The component effectively handles different states based on the props and project settings.

To improve accessibility, consider adding an aria-label to the CustomSelect component:

<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 "
  aria-label="Select cycle estimate type"
>
  {/* ... */}
</CustomSelect>

This will provide better context for screen readers and improve the overall accessibility of the component.


1-39: Great job on implementing the EstimateTypeDropdown component!

The component is well-structured, follows React best practices, and effectively handles different states and conditions. It provides a reusable solution for selecting cycle estimate types, which aligns with the PR objective of creating a common dropdown component.

To further enhance the component's reusability and maintainability, consider the following suggestions:

  1. Extract the cycleEstimateOptions into a separate constants file if it's used across multiple components. This would improve maintainability and ensure consistency.

  2. If this dropdown pattern is likely to be used for other types of selections in the future, consider creating a more generic TypeDropdown component that can be easily adapted for different use cases.

  3. Add unit tests to ensure the component behaves correctly under different prop combinations and project estimate settings.

These suggestions aim to improve the overall architecture and robustness of the component within the larger application context.

🧰 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)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a50a00e and b56b835.

📒 Files selected for processing (4)
  • web/ce/components/cycles/analytics-sidebar/base.tsx (2 hunks)
  • web/core/components/cycles/active-cycle/productivity.tsx (4 hunks)
  • web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (1 hunks)
  • web/core/components/cycles/dropdowns/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • web/core/components/cycles/dropdowns/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • web/core/components/cycles/active-cycle/productivity.tsx
🧰 Additional context used
🪛 Biome
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx

[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)

🔇 Additional comments (3)
web/ce/components/cycles/analytics-sidebar/base.tsx (3)

7-7: Import of 'Loader' component is appropriate

The addition of the Loader import from @plane/ui ensures that loading states are handled correctly within the component.


10-10: Replaced CustomSelect with EstimateTypeDropdown

Updating the import to use EstimateTypeDropdown aligns with the PR objective of introducing a common dropdown component for estimate types.


68-68: Implementation of EstimateTypeDropdown component

The EstimateTypeDropdown is correctly used with the appropriate props: showEstimateSelection, value, onChange, and projectId. This enhances the component by standardizing the estimate type selection process.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (2)

15-19: Simplify the isCurrentProjectEstimateEnabled assignment.

The current assignment uses an unnecessary ternary operator. It can be simplified to improve readability.

Consider applying this change:

-  const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : false;
+  const isCurrentProjectEstimateEnabled = !!(projectId && areEstimateEnabledByProjectId(projectId));

This change uses the double negation operator (!!) to convert the result directly to a boolean, making the code more concise.

🧰 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)


20-38: LGTM: Main rendering logic is well-implemented. Consider handling loading/error states.

The conditional rendering logic is sound and covers different scenarios effectively. The CustomSelect component is used appropriately, and the fallback rendering is handled correctly.

Consider adding explicit handling for loading and error states to improve user experience. For example:

if (isLoading) return <LoadingSpinner />;
if (error) return <ErrorMessage message="Failed to load estimate data" />;

// Rest of the component logic

This would provide better feedback to users when data is being fetched or if an error occurs.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b56b835 and 9893319.

📒 Files selected for processing (4)
  • web/ce/components/cycles/analytics-sidebar/base.tsx (3 hunks)
  • web/core/components/cycles/active-cycle/productivity.tsx (3 hunks)
  • web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (1 hunks)
  • web/core/store/cycle.store.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • web/ce/components/cycles/analytics-sidebar/base.tsx
  • web/core/components/cycles/active-cycle/productivity.tsx
🧰 Additional context used
🪛 Biome
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx

[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)

🔇 Additional comments (4)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (1)

1-13: LGTM: Imports and type definition are well-structured.

The imports are appropriate for the component's functionality, and the TProps type is well-defined with clear prop names and types. The onChange prop returning a Promise is a good practice for handling asynchronous updates.

web/core/store/cycle.store.ts (3)

2-2: LGTM: New import added correctly.

The isEmpty function from lodash has been imported correctly. This utility function will be useful for checking empty objects in the new getIsPointsDataAvailable method.


62-63: LGTM: Interface updated correctly.

The getIsPointsDataAvailable method has been properly added to the ICycleStore interface. The method signature matches the implementation, ensuring type consistency.


Line range hint 1-724: Overall assessment: Good addition with minor improvements needed.

The introduction of the getIsPointsDataAvailable method is a valuable addition to the CycleStore. It provides a way to check for the availability of points data across different cycle versions, which can be useful for rendering decisions in the UI or for data analysis.

The use of computedFn from mobx-utils is appropriate, as it will memoize the computed value based on the cycleId input, potentially improving performance for repeated calls with the same cycle.

To further improve the code:

  1. Consider adding a unit test for the new method to ensure it behaves correctly for different cycle versions and edge cases.
  2. Update any relevant documentation or comments to reflect this new functionality.
  3. Ensure that any components or other parts of the application that might need this information are updated to use this new method.

To verify the usage and impact of this new method, you can run the following script:

This script will help identify areas of the codebase that might benefit from or be affected by the new method.

✅ Verification successful

Overall assessment: Verification successful with no issues found.

The getIsPointsDataAvailable method has been properly integrated and is utilized across multiple components within the codebase. References to cycle.version and the usage of total_estimate_points and completion_chart are consistent with the intended functionality and do not introduce any conflicts or issues.

Recommendations:

  1. Unit Testing: Ensure comprehensive unit tests are in place for the getIsPointsDataAvailable method to cover different cycle versions and edge cases.
  2. Documentation: Update relevant documentation to include details about the new method and its usage.
  3. Performance Monitoring: Monitor the performance impact of the new method in components where it is used to ensure optimal performance.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential usage of the new method and related data structures

# Look for potential usage of the new method
echo "Potential usage of getIsPointsDataAvailable:"
rg "getIsPointsDataAvailable" --type typescript

# Check for references to cycle versions
echo "\nReferences to cycle versions:"
rg "cycle.version" --type typescript

# Look for places where estimate points or completion charts are used
echo "\nUsage of estimate points or completion charts:"
rg -e "total_estimate_points" -e "completion_chart" --type typescript

Length of output: 536


Script:

#!/bin/bash
# Description: Check for potential usage of the new method and related data structures

# Look for potential usage of the new method within TypeScript files
echo "Potential usage of getIsPointsDataAvailable:"
rg "getIsPointsDataAvailable" -g '*.ts' -g '*.tsx'

# Check for references to cycle versions within TypeScript files
echo "\nReferences to cycle versions:"
rg "cycle.version" -g '*.ts' -g '*.tsx'

# Look for places where estimate points or completion charts are used within TypeScript files
echo "\nUsage of estimate points or completion charts:"
rg -e "total_estimate_points" -e "completion_chart" -g '*.ts' -g '*.tsx'

Length of output: 4681

Comment on lines +1 to +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;
};
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)

Comment on lines 277 to 287
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;
}
});

@SatishGandham SatishGandham merged commit 645a261 into preview Oct 15, 2024
14 of 15 checks passed
@SatishGandham SatishGandham deleted the fix-cycle-chsrt-dropdown branch October 15, 2024 09:47
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