-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fixed relative date filter initalization #12811
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
Conversation
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 Summary
Fixed date filter initialization and display issues, focusing on relative date handling and proper pluralization.
- Modified
getRelativeDateDisplayValue
inpackages/twenty-front/src/modules/object-record/object-filter-dropdown/utils/getRelativeDateDisplayValue.ts
to ensure 'THIS' direction uses singular form (e.g., 'This week' instead of 'This weeks') - Updated
computeVariableDateViewFilterValue
inpackages/twenty-front/src/modules/views/view-filter-value/utils/computeVariableDateViewFilterValue.ts
to enforce amount=1 for 'THIS' direction and validate positive amounts - Enhanced
useApplyObjectFilterDropdownOperand
inpackages/twenty-front/src/modules/object-record/object-filter-dropdown/hooks/useApplyObjectFilterDropdownOperand.ts
to initialize relative filters with 'This day' by default
3 files reviewed, 3 comments
Edit PR Review Bot Settings | Greptile
} else if (amount === undefined || amount <= 0) { | ||
throw new Error( | ||
'Amount must be defined and greater than 0 for relative date filters', | ||
); | ||
} |
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.
style: Good validation, but error message could be more user-friendly. Consider extracting to a constant and making it less technical.
); | ||
} | ||
|
||
return `${direction}_${amount.toString()}_${unit}`; |
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.
style: toString() call unnecessary since template literals automatically convert to string. Remove for cleaner code.
return `${direction}_${amount.toString()}_${unit}`; | |
return `${direction}_${amount}_${unit}`; |
const defaultRelativeDate = { | ||
direction: 'THIS' as VariableDateViewFilterValueDirection, | ||
amount: 1, | ||
unit: 'DAY' as VariableDateViewFilterValueUnit, | ||
}; |
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.
style: Consider extracting defaultRelativeDate as a constant since it represents default configuration values
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:60504 This environment will automatically shut down when the PR is closed or after 5 hours. |
This PR fixes problems with date filter : - Filter chip shows the label with plural, ex : `This weeks` - Using a relative filter now initializes to `This day` - Switching between the different relative sub-operands (This, Past, Next) now initializes with a value so we're never in an "empty" state.
This PR fixes problems with date filter :
This weeks
This day