[WEB-3504]chore: issue properties refactor#6724
Conversation
WalkthroughThe changes update and consolidate constants, type definitions, and component structures related to issue display properties and spreadsheet views. New constants and a new component have been introduced to standardize spreadsheet property metadata and icons. The modifications include reformatting of type definitions, introduction of mappings for icons and columns, and removal of outdated constants. Changes span across constants, type declarations, and both CE and core component implementations to streamline how issue properties are represented and rendered. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (2)
web/core/components/issues/issue-layouts/utils.tsx (1)
711-721: Consider a fallback for missing icons.
WheniconKeyis not found inSpreadSheetPropertyIconMap, the component returnsnull. You could provide a default icon or a warning to help debugging.if (!Icon) - return null; + return <SomeDefaultIcon className="h-4 w-4" />;web/core/components/issues/issue-layouts/spreadsheet/columns/header-column.tsx (1)
53-53: Optional fallback for missing icons.
You rely onpropertyDetails.iconto decide which icon to display. If the key is invalid or unrecognized,SpreadSheetPropertyIconreturnsnull. Consider a fallback icon or a placeholder when icon metadata is unexpectedly missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/constants/src/issue/common.ts(3 hunks)packages/types/src/view-props.d.ts(2 hunks)web/ce/components/issues/issue-layouts/additional-properties.tsx(1 hunks)web/ce/components/issues/issue-layouts/utils.tsx(1 hunks)web/core/components/issues/issue-layouts/properties/all-properties.tsx(2 hunks)web/core/components/issues/issue-layouts/spreadsheet/columns/header-column.tsx(2 hunks)web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx(1 hunks)web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-view.tsx(1 hunks)web/core/components/issues/issue-layouts/utils.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (19)
packages/types/src/view-props.d.ts (1)
212-217: Well-structured new type for spreadsheet columns.The new
TSpreadsheetColumntype is well-defined with clear prop types that follow best practices. This is a good addition for stronger typing in the spreadsheet implementation.web/ce/components/issues/issue-layouts/additional-properties.tsx (1)
1-10: New component structure is good, but currently empty.The new
IssueAdditionalPropertiescomponent is well-structured with proper type definitions. However, it currently renders an empty fragment. Make sure to implement the actual rendering logic in a follow-up PR (if that's the plan) or complete the implementation.Is this component intentionally an empty placeholder for future implementation, or should it be rendering some content already?
web/core/components/issues/issue-layouts/properties/all-properties.tsx (2)
36-37: Good import organization with clear comment.The import follows best practices with appropriate comment labeling.
513-515: Clean integration of the new component.The
IssueAdditionalPropertiescomponent is integrated well with proper props being passed. The clear comment above helps developers understand its purpose in the component hierarchy.web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-view.tsx (1)
4-4: Good refactoring to use centralized constants.Moving from a local constant to an imported one from
@plane/constantsimproves code organization and maintainability. This is a positive change that aligns with good software engineering practices.web/core/components/issues/issue-layouts/utils.tsx (3)
3-3: Good adoption of React’s FC type.
This import ofFCclarifies the intent to use a typed functional component, which improves code readability and maintainability.
30-30: No issues with adding ISvgIcons.
Ensuring that icons share a common set of props throughISvgIconshelps maintain consistency across the codebase.
39-39:Details
❓ Verification inconclusive
Potential circular-import concern.
SpreadSheetPropertyIconMapis imported from the same folder as this file. Verify that this doesn’t create a circular reference.
🏁 Script executed:
#!/bin/bash # Description: Search for circular dependencies involving "issue-layouts/utils". # We'll look for any import lines from "issue-layouts/utils" in these files to confirm there's no cyc import. fd . --extension ts --extension tsx --exclude node_modules --exec rg 'from\s+\".*issue-layouts/utils\"'Length of output: 65613
Circular Dependency Concern – Please Verify Alias Resolution
I re-ran the search using a corrected regex to check for circular imports in the "issue-layouts/utils.tsx" file. There was no clear evidence of a circular dependency from the automated search. However, note that the file imports
SpreadSheetPropertyIconMapusing an alias (i.e."@/plane-web/components/issues/issue-layouts/utils"). Please manually verify that the alias correctly resolves to a location distinct from this file (and not a self‑reference) to ensure there isn’t an unintended circular dependency.• Verify the module alias mapping for
"@"to ensure it does not resolve back to the same file.
• Confirm that no other files create a dependency cycle involvingissue-layouts/utils.web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx (1)
9-11: Good reduction of imports into a single columns entry point.
ImportingSPREADSHEET_COLUMNSfrom one source centralizes the column definitions, eliminating duplicated code. The additional comment (“// utils”) is purely informational and not interfering with functionality.web/core/components/issues/issue-layouts/spreadsheet/columns/header-column.tsx (2)
3-5: No significant concerns with new icon imports.
These icons fromlucide-reactand the reference toSPREADSHEET_PROPERTY_DETAILSappear correctly integrated.
10-12: Centralized usage of UI components.
CombiningCustomMenu,Row, andSpreadSheetPropertyIconfrom shared modules keeps the code DRY.packages/constants/src/issue/common.ts (4)
1-1: Improved import organization.Consolidating the imports for
TIssueGroupByOptions,TIssueOrderByOptions, andIIssueDisplayPropertiesinto a single line improves readability and maintainability.
148-165: Well-structured property keys constant.The
ISSUE_DISPLAY_PROPERTIES_KEYSarray provides a centralized, type-safe list of all available issue display properties, which will help maintain consistency across the application.
214-229: Good separation of concerns for spreadsheet properties.The
SPREADSHEET_PROPERTY_LISTconstant creates a dedicated list specifically for spreadsheet views, making it clear which properties are available in this context.
231-353: Comprehensive property metadata structure.The
SPREADSHEET_PROPERTY_DETAILSconstant provides a well-organized mapping of property keys to their metadata, including internationalization titles, sorting options, and associated icons. This centralization will help maintain consistency across various UI components.web/ce/components/issues/issue-layouts/utils.tsx (4)
1-14: Good organization of icon imports.The imports are well-organized, making it easy to see which icons are being used in the component.
16-17: Enhanced type safety with additional type imports.Adding
IIssueDisplayPropertiesandTSpreadsheetColumntypes improves type checking and ensures consistency with the constants defined in the common file.
19-34: Comprehensive spreadsheet column component imports.The imports are well-organized and correspond directly to the column components used in the
SPREADSHEET_COLUMNSconstant.
55-70:Details
❓ Verification inconclusive
Well-structured spreadsheet column mapping.
The
SPREADSHEET_COLUMNSconstant provides a comprehensive mapping between property keys and their corresponding column components. This structure aligns well with theSPREADSHEET_PROPERTY_DETAILSconstant in the common file, creating a cohesive system for managing issue property representations.I'd recommend ensuring this component mapping is kept in sync with the
SPREADSHEET_PROPERTY_LISTandSPREADSHEET_PROPERTY_DETAILSconstants when properties are added or removed in the future.
🏁 Script executed:
#!/bin/bash # Verify that all properties in SPREADSHEET_PROPERTY_LIST have corresponding entries in SPREADSHEET_COLUMNS # Find all properties in SPREADSHEET_PROPERTY_LIST echo "Checking for consistency between SPREADSHEET_PROPERTY_LIST and SPREADSHEET_COLUMNS..." # Get the property list from common.ts property_list=$(rg -U "export const SPREADSHEET_PROPERTY_LIST.*?];" packages/constants/src/issue/common.ts --json | jq -r '.data.lines.text') echo "Property List found:" echo "$property_list" # Get the column mapping from utils.tsx column_mapping=$(rg -U "export const SPREADSHEET_COLUMNS.*?};" web/ce/components/issues/issue-layouts/utils.tsx --json | jq -r '.data.lines.text') echo "Column Mapping found:" echo "$column_mapping" # Compare to ensure all properties have corresponding column components echo "Checking for missing mappings..."Length of output: 694
Action: Verify Consistency Between Spreadsheet Property Declarations and Column Mappings
The mapping defined in
SPREADSHEET_COLUMNSis well-structured and aligns with our approach in managing issue property representations. However, please ensure that this mapping remains synchronized with the spreadsheet property declarations, particularly withSPREADSHEET_PROPERTY_LIST(and by extension,SPREADSHEET_PROPERTY_DETAILS). Our automated check did not retrieve any output for these constants, so it’s important to manually verify that:
- All properties listed in
SPREADSHEET_PROPERTY_LISTinpackages/constants/src/issue/common.tsare properly represented in theSPREADSHEET_COLUMNSmapping.- Any future additions or removals of properties are consistently updated in both the property list and the component mapping.
Overall, the mapping looks solid; just be cautious to update both sides when property changes occur.
|
Pull Request Linked with Plane Issues References
Comment Automatically Generated by Plane |
web/ce/components/issues/issue-layouts/additional-properties.tsx
Outdated
Show resolved
Hide resolved
vamsikrishnamathala
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web/ce/components/issues/issue-layouts/utils.tsx (1)
38-52:⚠️ Potential issueFix typos in icon map keys
There are typos in the icon map keys that could lead to incorrect icon rendering:
export const SpreadSheetPropertyIconMap: Record<string, FC<ISvgIcons>> = { Users: Users, - CalenderDays: CalendarDays, - CalenderCheck2: CalendarCheck2, + CalendarDays: CalendarDays, + CalendarCheck2: CalendarCheck2, Triangle: Triangle, Tag: Tag, DiceIcon: DiceIcon, ContrastIcon: ContrastIcon, Signal: Signal, CalendarClock: CalendarClock, DoubleCircleIcon: DoubleCircleIcon, Link2: Link2, Paperclip: Paperclip, LayersIcon: LayersIcon, };
🧹 Nitpick comments (1)
web/ce/components/issues/issue-layouts/utils.tsx (1)
37-69: Consider adding a SpreadSheetPropertyIcon componentFor consistency with the implementation pattern, consider adding a utility component to retrieve and render the appropriate icon based on the property name:
type TSpreadSheetPropertyIconProps = { iconKey: string; className?: string; }; export const SpreadSheetPropertyIcon: FC<TSpreadSheetPropertyIconProps> = ({ iconKey, className = "" }) => { const Icon = SpreadSheetPropertyIconMap[iconKey] || (() => null); return <Icon className={className} />; };This would make it easier to use the icons consistently throughout the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/ce/components/issues/issue-layouts/additional-properties.tsx(1 hunks)web/ce/components/issues/issue-layouts/utils.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
web/ce/components/issues/issue-layouts/additional-properties.tsx (1)
1-9: Component is exported but contains no implementationThe
IssueAdditionalPropertiescomponent is properly exported but currently renders an empty fragment. While this might be intentional for a staged refactoring approach, ensure that this is not an oversight and that implementation will be added in a future PR or commit.web/ce/components/issues/issue-layouts/utils.tsx (1)
54-69: LGTM! Comprehensive mapping of issue display properties to spreadsheet columnsThe
SPREADSHEET_COLUMNSconstant provides a complete mapping between issue display properties and their corresponding spreadsheet column components. This is a good approach for organizing the columns and will make maintenance easier.
web/ce/components/issues/issue-layouts/additional-properties.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/ce/components/issues/issue-layouts/additional-properties.tsx (1)
1-10: New component needs implementationThis component is currently just a skeleton with no implementation, returning an empty fragment. While this may be intentional as part of the refactoring process, ensure you add the required functionality in a subsequent PR or commit.
Consider enhancing this component with:
- JSDoc comments describing its purpose and usage
- Proper implementation that renders the issue properties based on displayProperties
import React, { FC } from "react"; import { IIssueDisplayProperties, TIssue } from "@plane/types"; +/** + * Component to display additional properties for work items based on the display settings + * @param displayProperties - Properties to be displayed + * @param issue - The issue/work item data to render properties from + */ export type TWorkItemLayoutAdditionalProperties = { displayProperties: IIssueDisplayProperties; issue: TIssue; }; -export const WorkItemLayoutAdditionalProperties: FC<TWorkItemLayoutAdditionalProperties> = (props) => <></>; +export const WorkItemLayoutAdditionalProperties: FC<TWorkItemLayoutAdditionalProperties> = ({ displayProperties, issue }) => { + // TODO: Implement rendering of additional properties based on displayProperties + return <></>; +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/ce/components/issues/issue-layouts/additional-properties.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
web/ce/components/issues/issue-layouts/additional-properties.tsx (2)
4-7: Type definition looks goodThe exported type definition is clear and properly structured with the necessary properties.
9-9: Consider implementing per a previous reviewThis component should be exported to be used with other components as previously suggested.
* chore: issue properties refactor * chore: added export to props * chore: updated component name
3 participants
Description
This update includes code refactor for Issue properties in List view, kanban view and spreadsheet view.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
WEB-3504
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor