-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Cards pipeline fix #4971
Cards pipeline fix #4971
Conversation
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Type: Enhancement
PR Summary: This pull request introduces the 'isCheckDate' feature across both the backend and frontend components of the cards plugin. The feature allows users to select a date after the card's creation date, enhancing the flexibility and usability of date-related functionalities within the cards system. Changes include the addition of the 'isCheckDate' field in various schemas and interfaces, updates to the UI components to accommodate the new feature, and adjustments to ensure the feature's integration is seamless across the system.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Ensure comprehensive testing across different time zones to verify the 'isCheckDate' functionality behaves as expected.
- Consider adding user documentation or tooltips in the UI to explain the 'isCheckDate' feature to end-users, enhancing their understanding and adoption of the new functionality.
- Review the consistency of the 'isCheckDate' feature's implementation across different parts of the application to ensure a uniform user experience.
packages/plugin-cards-ui/src/settings/boards/components/PipelineForm.tsx: Consider simplifying state update handlers for boolean values using a higher-order function to reduce redundancy and complexity.
The addition of isCheckDate
and related logic introduces additional complexity to the component, particularly in state management and method handling. While the functionality is necessary, we can simplify the implementation to improve maintainability. Consider using a higher-order function to generate handlers for boolean state updates, reducing the number of almost identical handler functions. Here's a suggested approach:
createBooleanStateHandler = (stateKey) => (e) => {
const isChecked = e.currentTarget.checked;
this.setState({ [stateKey]: isChecked });
};
onChangeIsCheckUser = this.createBooleanStateHandler('isCheckUser');
onChangeIsCheckDepartment = this.createBooleanStateHandler('isCheckDepartment');
onChangeIsCheckDate = this.createBooleanStateHandler('isCheckDate');
This method consolidates the logic for updating boolean states into a single, reusable function, reducing redundancy and making the component easier to understand and maintain.
packages/ui-cards/src/boards/components/editForm/CloseDate.tsx: Consider simplifying the component by extracting complex logic into utility functions and reducing state and prop complexity for improved maintainability.
While reviewing the proposed changes, I noticed an increase in complexity primarily due to the introduction of additional props (createdDate
, isCheckDate
) and more intricate date handling logic. This complexity can make the component harder to maintain and understand. Here are a few suggestions to simplify the code:
-
Extract Complex Date Logic: Consider moving the complex date comparison and validation logic into separate utility functions. This not only cleans up the component code but also makes these functions easier to test independently.
-
Simplify State Management: Evaluate if the number of states and props can be reduced. For instance,
isCheckDate
could potentially be derived from the presence ofcreatedDate
rather than being passed as a separate prop, reducing the component's state management complexity. -
Improve Variable Naming: More descriptive variable names could help clarify their purpose or how they're derived, enhancing code readability.
Here's a simplified code snippet that demonstrates these suggestions:
// Utility functions for date logic
function getMaxDate(dueDate, createdDate) {
return new Date(Math.max(new Date(dueDate).getTime(), new Date(createdDate).getTime()));
}
function isValidDate(current, createdDate, isCheckDate) {
return isCheckDate ? dayjs(current).isAfter(dayjs(createdDate).subtract(1, 'day')) : true;
}
// In the component
renderContent() {
const { reminderMinute, createdDate } = this.props;
const { dueDate } = this.state;
const isCheckDate = !!createdDate; // Derive isCheckDate based on createdDate presence
const validDate = isCheckDate ? getMaxDate(dueDate, createdDate) : dueDate;
const day = dayjs(validDate).format('YYYY-MM-DD');
const time = dayjs(dueDate).format('HH:mm');
// Use the utility function for date validation
const renderValidDate = (current) => isValidDate(current, createdDate, isCheckDate);
// Rest of the method remains the same...
}
These changes aim to reduce complexity, making the code easier to maintain and understand.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
* isCheckDate add on cards plugin api & ui * cards plugin close date isValidDate property add
isCheckDate add on cards plugin BE and FE
SELECT THE DAY AFTER THE CARD CREATED DATE