fix: moved dropdowns to chart component + added pending icon #5824
Conversation
WalkthroughThe changes in this pull request include the introduction of a new Changes
Suggested labels
Suggested reviewers
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
packages/ui/src/icons/pending-icon.tsx (1)
15-16: Use camelCase for JSX attributes.In React, HTML attributes should be written in camelCase. Please update the following attributes:
- fill-rule="evenodd" - clip-rule="evenodd" + fillRule="evenodd" + clipRule="evenodd"Apply this change to both path elements (lines 15-16 and 21-22).
Also applies to: 21-22
packages/ui/src/icons/index.ts (1)
34-34: LGTM! Consider maintaining alphabetical order.The addition of the pending icon export is correct and aligns with the PR objectives. This change makes the new pending icon component available for use throughout the application, which is great.
As a minor suggestion, consider maintaining the alphabetical order of exports in this file. You could move the new export statement to maintain consistency with the existing structure, placing it between "photo-filter-icon" and "planned-icon".
Here's a suggested placement for the new export:
export * from "./photo-filter-icon"; + export * from "./pending-icon"; export * from "./planned-icon";This small reorganization would enhance the file's readability and make it easier to locate specific icons in the future.
web/core/components/cycles/list/cycle-list-item-action.tsx (1)
214-214: Approve changes with a minor suggestion for readability.The modification to the button's visibility logic aligns well with the PR objective of moving dropdowns to the chart component. The implementation correctly hides the "More details" button when the cycle is active and the
peekCycleparameter is present in the URL.To improve readability, consider extracting the visibility condition into a separate variable:
const isButtonVisible = isMobile || (isActive && !searchParams.has("peekCycle")); // Then use it in the className className={`z-[1] flex text-custom-primary-200 text-xs gap-1 flex-shrink-0 ${ isButtonVisible ? "flex" : "hidden group-hover:flex" }`}This change would make the logic easier to understand at a glance and potentially easier to maintain in the future.
web/ce/components/cycles/analytics-sidebar/base.tsx (1)
39-39: Consider renaming the variable for clarityThe variable name
isCurrentEstimateTypeIsPointsis a bit verbose and can be simplified for better readability. Consider renaming it toisEstimateTypePoints.Apply this diff to fix:
- const isCurrentEstimateTypeIsPoints = estimateDetails && estimateDetails?.type === EEstimateSystem.POINTS; + const isEstimateTypePoints = estimateDetails && estimateDetails?.type === EEstimateSystem.POINTS;Remember to update all occurrences of this variable in the file.
web/core/components/cycles/analytics-sidebar/issue-progress.tsx (1)
28-32: Define theoptionstype before its usageThe
optionstype is used incycleEstimateOptionsbefore it is defined later in the code. Defining the type before its usage can enhance readability and make the code easier to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/ui/src/icons/index.ts (1 hunks)
- packages/ui/src/icons/pending-icon.tsx (1 hunks)
- web/ce/components/cycles/analytics-sidebar/base.tsx (3 hunks)
- web/core/components/cycles/analytics-sidebar/issue-progress.tsx (4 hunks)
- web/core/components/cycles/list/cycle-list-item-action.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
web/ce/components/cycles/analytics-sidebar/base.tsx
[error] 36-36: 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 (1)
packages/ui/src/icons/pending-icon.tsx (1)
1-5: LGTM: Imports and component declaration are well-structured.The import statements and component declaration are correctly implemented. The use of the
ISvgIconsinterface for props promotes type safety, and providing default values forwidthandheightis a good practice.
| <svg | ||
| width={width} | ||
| height={height} | ||
| viewBox="0 0 24 24" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| className={className} | ||
| > | ||
| <path | ||
| fill-rule="evenodd" | ||
| clip-rule="evenodd" | ||
| d="M12 3C7.02944 3 3 7.02944 3 12C3 16.9706 7.02944 21 12 21C16.9706 21 21 16.9706 21 12C21 7.02944 16.9706 3 12 3ZM1 12C1 5.92487 5.92487 1 12 1C18.0751 1 23 5.92487 23 12C23 18.0751 18.0751 23 12 23C5.92487 23 1 18.0751 1 12Z" | ||
| fill="#455068" | ||
| /> | ||
| <path | ||
| fill-rule="evenodd" | ||
| clip-rule="evenodd" | ||
| d="M12 5C12.5523 5 13 5.44772 13 6V11.382L16.4472 13.1056C16.9412 13.3526 17.1414 13.9532 16.8944 14.4472C16.6474 14.9412 16.0468 15.1414 15.5528 14.8944L11.5528 12.8944C11.214 12.725 11 12.3788 11 12V6C11 5.44772 11.4477 5 12 5Z" | ||
| fill="#455068" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the icon color customizable.
The SVG implementation looks good and should correctly render a pending state icon. However, the fill color is currently hardcoded as "#455068". To improve the icon's reusability across different color schemes, consider using the color prop to set the fill color dynamically.
Here's a suggested modification:
- fill="#455068"
+ fill={color || "#455068"}Apply this change to both path elements (lines 18 and 24).
📝 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.
| <svg | |
| width={width} | |
| height={height} | |
| viewBox="0 0 24 24" | |
| fill="none" | |
| xmlns="http://www.w3.org/2000/svg" | |
| className={className} | |
| > | |
| <path | |
| fill-rule="evenodd" | |
| clip-rule="evenodd" | |
| d="M12 3C7.02944 3 3 7.02944 3 12C3 16.9706 7.02944 21 12 21C16.9706 21 21 16.9706 21 12C21 7.02944 16.9706 3 12 3ZM1 12C1 5.92487 5.92487 1 12 1C18.0751 1 23 5.92487 23 12C23 18.0751 18.0751 23 12 23C5.92487 23 1 18.0751 1 12Z" | |
| fill="#455068" | |
| /> | |
| <path | |
| fill-rule="evenodd" | |
| clip-rule="evenodd" | |
| d="M12 5C12.5523 5 13 5.44772 13 6V11.382L16.4472 13.1056C16.9412 13.3526 17.1414 13.9532 16.8944 14.4472C16.6474 14.9412 16.0468 15.1414 15.5528 14.8944L11.5528 12.8944C11.214 12.725 11 12.3788 11 12V6C11 5.44772 11.4477 5 12 5Z" | |
| fill="#455068" | |
| /> | |
| <svg | |
| width={width} | |
| height={height} | |
| viewBox="0 0 24 24" | |
| fill="none" | |
| xmlns="http://www.w3.org/2000/svg" | |
| className={className} | |
| > | |
| <path | |
| fill-rule="evenodd" | |
| clip-rule="evenodd" | |
| d="M12 3C7.02944 3 3 7.02944 3 12C3 16.9706 7.02944 21 12 21C16.9706 21 21 16.9706 21 12C21 7.02944 16.9706 3 12 3ZM1 12C1 5.92487 5.92487 1 12 1C18.0751 1 23 5.92487 23 12C23 18.0751 18.0751 23 12 23C5.92487 23 1 18.0751 1 12Z" | |
| fill={color || "#455068"} | |
| /> | |
| <path | |
| fill-rule="evenodd" | |
| clip-rule="evenodd" | |
| d="M12 5C12.5523 5 13 5.44772 13 6V11.382L16.4472 13.1056C16.9412 13.3526 17.1414 13.9532 16.8944 14.4472C16.6474 14.9412 16.0468 15.1414 15.5528 14.8944L11.5528 12.8944C11.214 12.725 11 12.3788 11 12V6C11 5.44772 11.4477 5 12 5Z" | |
| fill={color || "#455068"} | |
| /> |
| @@ -29,41 +33,82 @@ 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 = projectId && areEstimateEnabledByProjectId(projectId) ? true : false; | |||
There was a problem hiding this comment.
Simplify unnecessary boolean ternary operator
The ternary operator in line 36 is unnecessary because the expression already evaluates to a boolean value. You can simplify the code by removing the ternary operator.
Apply this diff to fix:
- 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.
| const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : false; | |
| const isCurrentProjectEstimateEnabled = Boolean(projectId && areEstimateEnabledByProjectId(projectId)); |
🧰 Tools
🪛 Biome
[error] 36-36: 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 const cycleEstimateOptions: options[] = [ | ||
| { value: "issues", label: "issues" }, | ||
| { value: "points", label: "points" }, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming options type to Options for consistency
The type options is currently in lowercase. In TypeScript, it's conventional to use PascalCase for type and interface names. Renaming options to Options will improve code readability and maintain consistency throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
web/core/components/cycles/quick-actions.tsx (1)
118-118: Approved: Grammar correction improves clarity.The change from "cycle" to "cycles" correctly fixes the grammatical error in the description. This improvement enhances the clarity and professionalism of the user interface.
Consider capitalizing the first letter of the description for consistency with other UI text:
- description: isCompleted ? undefined : "Only completed cycles can\nbe archived.", + description: isCompleted ? undefined : "Only completed cycles can\nbe archived.",web/core/components/cycles/analytics-sidebar/issue-progress.tsx (2)
63-63: LGTM: Good addition of cycle-related functions.The inclusion of
getPlotTypeByCycleIdandgetEstimateTypeByCycleIdfrom theuseCyclehook is a good change. It centralizes the logic for retrieving cycle-specific data, which aligns with the removal of the estimate type selector.Consider adding a comment explaining the purpose of these newly added functions for better code readability.
Line range hint
160-173: LGTM: Simplified rendering of chart components.The changes to the JSX structure, particularly the simplified rendering of
SidebarChartRootandCycleProgressStats, align well with the removal of the estimate type selector and progress percentage display. The addition ofplotTypeas a prop toCycleProgressStatsis consistent with earlier changes.Consider adding a comment explaining the significance of
chartDistributionDataand why theCycleProgressStatscomponent is conditionally rendered based on its presence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
- web/app/[workspaceSlug]/(projects)/active-cycles/header.tsx (1 hunks)
- web/core/components/cycles/analytics-sidebar/issue-progress.tsx (4 hunks)
- web/core/components/cycles/analytics-sidebar/sidebar-header.tsx (1 hunks)
- web/core/components/cycles/quick-actions.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- web/app/[workspaceSlug]/(projects)/active-cycles/header.tsx
- web/core/components/cycles/analytics-sidebar/sidebar-header.tsx
🧰 Additional context used
🔇 Additional comments (2)
web/core/components/cycles/analytics-sidebar/issue-progress.tsx (2)
10-10: LGTM: Import and type definition changes look good.The addition of
TCyclePlotTypeandTProgressSnapshotimports, along with the redefinition ofcycleEstimateOptions, align well with the changes in the component logic. These modifications improve type safety and simplify the options structure.Also applies to: 28-31
Line range hint
48-51: Consider renamingoptionstype toOptionsfor consistencyAs previously suggested, it's recommended to rename the
optionstype toOptionsusing PascalCase. This change would improve code readability and maintain consistency with TypeScript naming conventions for types and interfaces.
|
|
||
| import { ISvgIcons } from "./type"; | ||
|
|
||
| export const PendingState: React.FC<ISvgIcons> = ({ width = "10", height = "11", className, color }) => ( |
There was a problem hiding this comment.
Set #455068 as the default color instead of hardcoding, so that we can override if required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/core/components/cycles/analytics-sidebar/issue-progress.tsx (2)
Line range hint
159-173: LGTM! Component simplification aligns with PR objectives.The modifications to the
CycleAnalyticsProgresscomponent successfully simplify its logic by moving the estimate type selection to the chart component. This change improves the separation of concerns and aligns with the PR objectives.Consider adding a brief comment above the
SidebarChartRootcomponent to explain its purpose or any important props it receives. This would enhance code readability for future maintainers.+ // Render the main cycle analytics chart <SidebarChartRoot workspaceSlug={workspaceSlug} projectId={projectId} cycleId={cycleId} />
Line range hint
1-179: Overall, excellent refactoring that improves component structure and separation of concerns.The changes in this file successfully achieve the PR objectives by:
- Moving dropdown functionality to the chart component.
- Simplifying the
CycleAnalyticsProgresscomponent.- Improving type definitions and constant exports.
These modifications enhance the maintainability and readability of the code. The separation of concerns is now clearer, with the
SidebarChartRootcomponent handling more of the chart-specific logic.As the codebase evolves, consider creating a separate file for shared types and constants related to cycle analytics. This would further improve modularity and reusability across different components dealing with cycle data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/ui/src/icons/pending-icon.tsx (1 hunks)
- web/ce/components/cycles/analytics-sidebar/base.tsx (3 hunks)
- web/core/components/cycles/analytics-sidebar/issue-progress.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/icons/pending-icon.tsx
- web/ce/components/cycles/analytics-sidebar/base.tsx
🧰 Additional context used
🔇 Additional comments (2)
web/core/components/cycles/analytics-sidebar/issue-progress.tsx (2)
27-39: LGTM! New type and constants are well-defined.The new
Optionstype and the exported constantscycleEstimateOptionsandcycleChartOptionsare well-defined and follow TypeScript conventions. The naming is consistent and clear.Note: This change addresses the previous suggestion to rename
optionstoOptionsfor consistency.
10-10: LGTM! Import and hook usage updates are appropriate.The changes to the import statement and the
useCyclehook usage are consistent with the modifications in the component. The removal of unused functions from the hook destructuring improves code cleanliness.Also applies to: 62-62
Summary
Summary by CodeRabbit
New Features
PendingStateicon component for improved user feedback.SidebarChartcomponent with new estimate handling features and a user-friendly selection interface.CycleListItemActioncomponent for better responsiveness based on user context.Bug Fixes
issue-progresscomponent for improved performance.Refactor