-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix: disable endDate for created-at, updated-at, formula #9915
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
📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
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.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/nc-gui/components/smartsheet/toolbar/Calendar/Range.vue (2)
282-282: Verify disabled state conditionsThe disabled state for the end date selector combines multiple conditions. Consider extracting these conditions into a computed property for better maintainability.
+const isEndDateDisabled = computed(() => !range.fk_from_column_id || isLocked || isDisabled) - :disabled="!range.fk_from_column_id || isLocked || isDisabled" + :disabled="isEndDateDisabled"
254-321: Consider unifying EE and non-EE UI behaviorsThe current implementation shows different behaviors for EE and non-EE versions:
- Non-EE: Shows a button with tooltip
- EE: Shows a select dropdown
Consider unifying these behaviors to maintain consistency across versions, while still respecting the disabled state for specific field types.
packages/nc-gui/components/dlg/ViewCreate.vue (2)
538-540: Refactor condition for readability and maintainabilityConsider simplifying the condition within the
isDisabledcomputed property for better readability:Apply this diff to refactor the code:
- [UITypes.DateTime, UITypes.CreatedTime, UITypes.LastModifiedTime, UITypes.Formula].includes( - viewSelectFieldOptions.value.find((f) => f.value === form.calendar_range[0]?.fk_from_column_id)?.uidt, - ) && !isRangeEnabled.value + const selectedUidt = viewSelectFieldOptions.value.find( + (f) => f.value === form.calendar_range[0]?.fk_from_column_id + )?.uidt + const disabledUidts = [ + UITypes.DateTime, + UITypes.CreatedTime, + UITypes.LastModifiedTime, + UITypes.Formula, + ] + return disabledUidts.includes(selectedUidt) && !isRangeEnabled.valueThis refactor extracts variables and arrays for clarity.
1059-1059: Remove duplicate line numberingIt appears that line 1059 is numbered twice in the annotations. This may cause confusion when referencing specific lines in the code review.
Please adjust the line numbering to reflect the correct lines for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/nc-gui/components/dlg/ViewCreate.vue(2 hunks)packages/nc-gui/components/smartsheet/toolbar/Calendar/Range.vue(2 hunks)
🔇 Additional comments (2)
packages/nc-gui/components/smartsheet/toolbar/Calendar/Range.vue (1)
148-150: LGTM: Correctly implements end date disabling for specified field types
The implementation properly extends the disabled state to include CreatedTime, LastModifiedTime, and Formula field types, which aligns with the PR objectives.
packages/nc-gui/components/dlg/ViewCreate.vue (1)
538-540: Ensure isDisabled logic correctly reflects feature availability
The condition for isDisabled now includes additional UITypes (UITypes.CreatedTime, UITypes.LastModifiedTime, UITypes.Formula). Verify that this change aligns with the intended functionality of disabling the calendar range selection for these field types.
To confirm that the logic correctly disables the calendar range when necessary, ensure that the viewSelectFieldOptions contain the expected uidt values and that isRangeEnabled reflects the feature toggle accurately.
|
Uffizzi Preview |
Change Summary
Disable end Date support for createdAt, updatedAt, Formula
Change type