Skip to content
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

chore: add wds datepicker widget #37711

Merged
merged 6 commits into from
Nov 29, 2024
Merged

Conversation

jsartisan
Copy link
Contributor

@jsartisan jsartisan commented Nov 26, 2024

CleanShot 2024-11-26 at 15 56 15

/ok-to-test tags="@tag.Anvil"

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced the WDS Date Picker Widget, enhancing date selection capabilities within the UI.
    • Added configuration options for widget size, visibility, and autocomplete behavior.
    • Implemented comprehensive validation for date input, ensuring accurate user selections.
    • Expanded widget collection to include the new WDS Date Picker Widget.
    • Introduced new constants for date format options, facilitating diverse formatting choices.
    • Added support for a new "Date" input type, enhancing input widget configurability.
  • Documentation

    • Updated property pane configurations to include detailed settings for date format, validation, and event handling.
  • Bug Fixes

    • Improved handling of derived properties to ensure proper context during widget interactions.

These updates collectively improve user experience and flexibility in date selection within the application.

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12083004714
Commit: b17348e
Cypress dashboard.
Tags: @tag.Anvil
Spec:


Fri, 29 Nov 2024 10:44:39 UTC

@jsartisan jsartisan requested a review from a team as a code owner November 26, 2024 10:25
@jsartisan jsartisan requested review from jacquesikot and removed request for a team November 26, 2024 10:25
Copy link
Contributor

coderabbitai bot commented Nov 26, 2024

Walkthrough

This pull request introduces a comprehensive set of configuration objects and components for a new date picker widget in the UI builder. Key additions include anvilConfig, autocompleteConfig, defaultsConfig, metaConfig, methodsConfig, settersConfig, and propertyPaneContentConfig, among others. The changes also encompass a new React component WDSDatePickerWidget that integrates these configurations, along with validation logic for date selection. Test cases for the validation function and updates to widget maps and utility functions are also included.

Changes

File Change Summary
.../WDSDatePickerWidget/config/anvilConfig.ts Added anvilConfig object with properties for widget size and visibility.
.../WDSDatePickerWidget/config/autocompleteConfig.ts Introduced autocompleteConfig for datepicker behavior, including visibility and date formatting.
.../WDSDatePickerWidget/config/defaultsConfig.ts Created defaultsConfig defining default properties for the date picker widget.
.../WDSDatePickerWidget/config/metaConfig.ts Added metaConfig for widget identity and categorization.
.../WDSDatePickerWidget/config/methodsConfig.ts Introduced methodsConfig with icon and thumbnail component references.
.../WDSDatePickerWidget/config/propertyPaneConfig/contentConfig.ts Added propertyPaneContentConfig for property pane configuration of the date picker.
.../WDSDatePickerWidget/config/settersConfig.ts Created settersConfig for managing visibility and disabled states.
.../WDSDatePickerWidget/constants.ts Added DATE_FORMAT_OPTIONS for various date formats.
.../WDSDatePickerWidget/index.ts Introduced index file for easier imports of WDSDatePickerWidget.
.../WDSDatePickerWidget/widget/derived.js Added isValid method for date validation logic.
.../WDSDatePickerWidget/widget/derived.test.ts Created test suite for isValid function covering various validation scenarios.
.../WDSDatePickerWidget/widget/helpers.ts Introduced validateInput function for input validation feedback.
.../WDSDatePickerWidget/widget/index.tsx Added WDSDatePickerWidget React component with configuration methods and event handling.
.../WDSDatePickerWidget/widget/types.ts Created WDSDatePickerWidgetProps interface defining required properties for the widget.
.../constants.ts Updated WDS_V2_WIDGET_MAP to include WDS_DATEPICKER_WIDGET.
.../WidgetUtils.ts Added parseDerivedProperties function for handling derived properties.
.../index.ts Updated to include WDSDatePickerWidget in widget exports.

Possibly related PRs

Suggested labels

Enhancement, Widgets Product, Date Picker Widget, Anvil Pod, Task

Suggested reviewers

  • ApekshaBhosale
  • sagar-qa007
  • KelvinOm

🎉 In the land of code where widgets play,
A date picker blooms, brightening the day.
With configs galore and tests all around,
Our UI's now richer, in features profound!
So let's raise a toast to this wonderful feat,
For a date picker widget that's truly a treat! 🎈


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ff38b36 and b17348e.

📒 Files selected for processing (3)
  • app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/config/defaultsConfig.ts (2 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/defaultsConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/config/defaultsConfig.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/defaultsConfig.ts
🔇 Additional comments (4)
app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/config/defaultsConfig.ts (2)

1-4: LGTM: Clean import structure

The addition of INPUT_TYPES import is well-organized and properly scoped.


19-19: Verify consistent inputType usage across widgets

The addition of inputType follows the widget system pattern. Let's verify this pattern across other widgets.

✅ Verification successful

Let me verify if there are any other input widgets in the WDS directory that might be missing the inputType.


Let me check specifically for input-type widgets and their inputType definitions.


inputType implementation is consistent with widget system pattern

The addition of inputType: INPUT_TYPES.PHONE_NUMBER follows the established pattern across WDS input widgets. All input-type widgets consistently define their inputType:

  • PhoneInput: INPUT_TYPES.PHONE_NUMBER
  • CurrencyInput: INPUT_TYPES.CURRENCY
  • Base Input: "TEXT"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent inputType usage across WDS widgets
# Expected: All WDS input widgets should define inputType in their defaultsConfig

# Search for defaultsConfig in WDS widgets
rg -l "defaultsConfig" --glob "app/client/src/modules/ui-builder/ui/wds/*Widget/config/defaultsConfig.ts" | \
  xargs rg "inputType:\s*INPUT_TYPES\." -A 1

Length of output: 869


Script:

#!/bin/bash
# Find all WDS input-related widgets
fd "WDS.*Input.*Widget|WDSTextWidget" "app/client/src/modules/ui-builder/ui/wds" -t d

# Then check their defaultsConfig for inputType
rg -l "defaultsConfig" --glob "app/client/src/modules/ui-builder/ui/wds/*Widget/config/defaultsConfig.ts" | \
  xargs cat

Length of output: 17644


Script:

#!/bin/bash
# Search for all input-related widgets and their inputType definitions
rg "inputType:" --glob "app/client/src/modules/ui-builder/ui/wds/WDS*Input*Widget/config/defaultsConfig.ts" -B 2 -A 2

Length of output: 1618

app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/config/defaultsConfig.ts (2)

1-4: LGTM! Clean import structure

The imports are well-organized and properly scoped from the WDSBaseInputWidget module.


20-20: LGTM! Appropriate input type configuration

The inputType property is correctly set to CURRENCY, maintaining consistency with the widget's purpose.

Let's verify that this input type is properly defined:

✅ Verification successful

Confirmed: INPUT_TYPES.CURRENCY is properly defined

The CURRENCY input type is correctly defined in the WDSBaseInputWidget constants and properly mapped to the WDS_CURRENCY_INPUT_WIDGET type, validating its usage in the defaultsConfig.ts file.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify INPUT_TYPES.CURRENCY is properly defined
ast-grep --pattern 'const INPUT_TYPES = {
  $$$
  CURRENCY: $_,
  $$$
}'

Length of output: 74


Script:

#!/bin/bash
# Let's try a broader search for INPUT_TYPES definition
rg "INPUT_TYPES.*=.*{" -A 10

Length of output: 128


Script:

#!/bin/bash
# Also search for any imports of INPUT_TYPES
rg "import.*INPUT_TYPES"

Length of output: 1425


Script:

#!/bin/bash
# Search for files containing CURRENCY as a type
rg "CURRENCY.*:" --type ts

Length of output: 4883


Script:

#!/bin/bash
# Let's check the actual definition of INPUT_TYPES in WDSBaseInputWidget constants
cat "app/client/src/modules/ui-builder/ui/wds/WDSBaseInputWidget/constants.ts"

Length of output: 776


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Nov 26, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🧹 Outside diff range and nitpick comments (17)
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/settersConfig.ts (2)

1-12: Consider adding date-specific setters.

While the basic visibility and disabled setters are correctly implemented, a date picker typically needs setters for date-related operations (e.g., setMinDate, setMaxDate, setDateFormat).

Consider adding these common date picker setters:

 export const settersConfig = {
   __setters: {
     setVisibility: {
       path: "isVisible",
       type: "boolean",
     },
     setDisabled: {
       path: "isDisabled",
       type: "boolean",
     },
+    setMinDate: {
+      path: "minDate",
+      type: "string",
+    },
+    setMaxDate: {
+      path: "maxDate",
+      type: "string",
+    },
+    setDateFormat: {
+      path: "dateFormat",
+      type: "string",
+    },
   },
 };

3-10: Consider adding type validation functions.

The boolean type declaration is good, but adding validation functions would provide runtime type safety and transformation capabilities.

Consider enhancing the type safety:

     setVisibility: {
       path: "isVisible",
       type: "boolean",
+      validate: (value: unknown) => typeof value === "boolean",
+      transform: (value: unknown) => Boolean(value),
     },
     setDisabled: {
       path: "isDisabled",
       type: "boolean",
+      validate: (value: unknown) => typeof value === "boolean",
+      transform: (value: unknown) => Boolean(value),
     },
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/metaConfig.ts (1)

6-13: Consider enhancing searchTags and clarifying needsMeta.

  1. Consider adding more search terms like "calendar", "datetime", "schedule" to improve widget discoverability.
  2. The purpose of needsMeta: true should be documented for maintainability.
  needsMeta: true, // Add comment explaining the purpose
  searchTags: [
    "date",
    "picker",
    "date picker",
    "date time",
    "date time picker",
+   "calendar",
+   "datetime",
+   "schedule"
  ],
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/types.ts (1)

3-11: Add JSDoc documentation for the interface and its properties

The interface would benefit from proper documentation explaining the purpose of each property and any format requirements.

Add documentation like this:

+ /**
+  * Props interface for the WDS Date Picker Widget
+  */
 export interface WDSDatePickerWidgetProps extends WidgetProps {
+   /** Selected date value in ISO format */
    selectedDate: string;
+   /** Default date value in ISO format */
    defaultDate: string;
+   /** Callback triggered when date is selected */
    onDateSelected: string;
+   /** Whether the date picker requires a value */
    isRequired?: boolean;
+   /** Whether the date picker is disabled */
    isDisabled?: boolean;
+   /** Label text for the date picker */
    label: string;
+   /** Optional tooltip text for the label */
    labelTooltip?: string;
 }
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/autocompleteConfig.ts (1)

3-11: Consider adding more essential widget properties.

The configuration looks good but seems minimal compared to standard datepicker capabilities. Consider adding properties for:

  • minDate and maxDate for date range validation
  • dateFormat for format configuration
  • timezone for timezone handling

Example addition:

 export const autocompleteConfig = {
   "!doc":
     "Datepicker is used to capture the date and time from a user. It can be used to filter data base on the input date range as well as to capture personal information such as date of birth",
   "!url": "https://docs.appsmith.com/widget-reference/datepicker",
   isVisible: DefaultAutocompleteDefinitions.isVisible,
   selectedDate: "string",
   formattedDate: "string",
   isDisabled: "bool",
+  minDate: "string",
+  maxDate: "string",
+  dateFormat: "string",
+  timezone: "string",
 };
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/defaultsConfig.ts (2)

4-18: Consider using proper TypeScript interface instead of type assertion

The double type assertion (as unknown as WidgetDefaultProps) is a potential type-safety concern. Consider defining an interface that extends WidgetDefaultProps with the expected shape.

+interface DatePickerDefaultProps extends WidgetDefaultProps {
+  dateFormat: string;
+  timePrecision: string;
+  isInline: boolean;
+  // ... other specific properties
+}
+
-export const defaultsConfig = {
+export const defaultsConfig: DatePickerDefaultProps = {
   // ... current properties
-} as unknown as WidgetDefaultProps;
+};

7-7: Consider using ISO 8601 format as default

While "YYYY-MM-DD HH:mm" is readable, consider using the ISO 8601 format ("YYYY-MM-DDTHH:mm:ss.SSSZ") for better international compatibility.

app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.test.ts (3)

1-5: Consider adding type safety to mockMoment function

The mockMoment helper function could benefit from stricter typing to prevent potential runtime errors.

-const mockMoment = (date: string) => moment(date);
+const mockMoment = (date: string): moment.Moment => moment(date);

7-23: Add test cases for invalid date strings

The basic validation tests look good, but consider adding test cases for invalid date strings to ensure robust validation.

Example test case to add:

it("should return false when selectedDate is an invalid date string", () => {
  const props = { isDirty: true, selectedDate: "invalid-date" };
  expect(derived.isValid(props, mockMoment)).toBe(false);
});

25-54: Add boundary value tests for date ranges

Consider adding tests for edge cases where selectedDate exactly equals minDate or maxDate.

Example test cases to add:

it("should return true when selectedDate equals minDate", () => {
  const props = {
    isDirty: true,
    minDate: "2023-01-01",
    selectedDate: "2023-01-01",
  };
  expect(derived.isValid(props, mockMoment)).toBe(true);
});

it("should return true when selectedDate equals maxDate", () => {
  const props = {
    isDirty: true,
    maxDate: "2023-12-31",
    selectedDate: "2023-12-31",
  };
  expect(derived.isValid(props, mockMoment)).toBe(true);
});
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/constants.ts (3)

4-84: Optimize date format initialization

The current implementation creates multiple moment instances during module initialization. Consider using a single reference date for all formats.

+const referenceDate = new Date();
 export const DATE_FORMAT_OPTIONS = [
   {
-    label: moment().format("YYYY-MM-DDTHH:mm:ss.sssZ"),
+    label: format(referenceDate, "yyyy-MM-dd'T'HH:mm:ss.SSSxxx"),
     subText: "ISO 8601",
-    value: "YYYY-MM-DDTHH:mm:ss.sssZ",
+    value: "yyyy-MM-dd'T'HH:mm:ss.SSSxxx",
   },
   // ... apply similar changes to other formats
 ];

85-88: Add TypeScript type definitions

Consider adding explicit type definitions for the format options to improve type safety and developer experience.

interface DateFormatOption {
  label: string;
  subText: string;
  value: string;
  subTextPosition: SubTextPosition;
}

export const DATE_FORMAT_OPTIONS: DateFormatOption[] = [
  // ... existing options
].map((x) => ({
  ...x,
  subTextPosition: SubTextPosition.BOTTOM,
}));

4-88: Consider adding i18n support

The current implementation might not handle international date formats well. Consider integrating with the application's i18n system for localized date formats.

app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/contentConfig.ts (3)

1-3: Consider using absolute imports consistently

The relative import ../../constants could be changed to use absolute paths for better maintainability.

import { ValidationTypes } from "constants/WidgetValidation";
-import { DATE_FORMAT_OPTIONS } from "../../constants";
+import { DATE_FORMAT_OPTIONS } from "modules/ui-builder/ui/wds/constants";

61-67: Consider using an enum for timePrecision values

The allowed values for timePrecision are hardcoded. Consider defining an enum to improve maintainability and type safety.

enum TimePrecision {
  Day = "day",
  Hour = "hour",
  Minute = "minute",
  Second = "second"
}

// Then use it in validation
validation: {
  type: ValidationTypes.TEXT,
  params: {
    allowedValues: Object.values(TimePrecision),
    default: TimePrecision.Day,
  },
}

157-166: Enhance loading animation documentation

The helpText for animateLoading could be more descriptive about when the animation is triggered and its visual appearance.

-helpText: "Controls the loading of the widget",
+helpText: "Shows a loading state with animation when the widget is processing or fetching data",
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx (1)

121-125: Simplify the parseDate method

You can simplify the parseDate method by eliminating the unnecessary use of moment.

Apply this diff:

-  private parseDate(date: string | undefined) {
-    return date
-      ? parseDateTime(moment(date).format("YYYY-MM-DDTHH:mm:ss"))
-      : undefined;
-  }
+  private parseDate(date: string | undefined) {
+    return date ? parseDateTime(date) : undefined;
+  }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8cb0bee and 93c8a9e.

📒 Files selected for processing (19)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/anvilConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/autocompleteConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/defaultsConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/index.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/metaConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/methodsConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/contentConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/index.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/settersConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/constants.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/index.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.js (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.test.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/helpers.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/types.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/constants.ts (1 hunks)
  • app/client/src/widgets/WidgetUtils.ts (1 hunks)
  • app/client/src/widgets/index.ts (2 hunks)
✅ Files skipped from review due to trivial changes (4)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/index.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/methodsConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/index.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/index.ts
🧰 Additional context used
📓 Learnings (1)
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/anvilConfig.ts (1)
Learnt from: riodeuno
PR: appsmithorg/appsmith#30351
File: app/client/src/widgets/wds/WDSModalWidget/config/anvilConfig.ts:1-6
Timestamp: 2024-11-12T08:11:43.278Z
Learning: The `anvilConfig` is optional for widgets that do not participate in the main container's layout flow, such as the modal widget in the current context.
🔇 Additional comments (17)
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/anvilConfig.ts (2)

1-1: LGTM!

Clean import of the required type.


3-11: Verify the sizing token and breakpoint value.

The configuration looks good, but let's verify:

  1. Is "sizing-30" a valid token in the design system?
  2. Is the 180px breakpoint aligned with the design system's breakpoint scale?
✅ Verification successful

The previous results show consistent usage of "sizing-30" and "180px" across multiple WDS widgets, but we need to verify these values against the design system's token definitions and breakpoint scales to ensure they're valid.


The previous results show that "sizing-30" is used consistently across WDS widgets and is defined in the design system's dimensions file, but we need to verify its exact definition and the breakpoint value in the design system tokens.


The sizing token and breakpoint values are valid and consistently used across WDS widgets.

The verification shows:

  1. "sizing-30" is a valid token defined in the design system's dimensions and used consistently across multiple WDS widgets
  2. The "180px" breakpoint is a standard breakpoint value used consistently in WDS widget configurations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other usages of the sizing token and breakpoint
rg -g '*.{ts,tsx}' '"sizing-30"' 
rg -g '*.{ts,tsx}' '"180px".*breakpoint|breakpoint.*"180px"'

# Look for design system token definitions
fd -e ts -e tsx -e js -e json -E '*test*' -E '*spec*' . | xargs rg 'sizing-30|SIZING_30|Sizing30'

Length of output: 3243


Script:

#!/bin/bash
# Let's check the design system's token definitions and breakpoint scales
fd -g '*tokens*.{ts,tsx,js}' -g '*theme*.{ts,tsx,js}' -g '*breakpoint*.{ts,tsx,js}' | xargs rg -A 5 'sizing|breakpoint'

# Also check if there's any documentation about sizing or breakpoints
fd -g 'README.md' -g '*.mdx' -g '*.md' | xargs rg -i 'sizing|breakpoint'

Length of output: 123341


Script:

#!/bin/bash
# Let's check the design system's token definitions specifically
fd -g '*tokens*.{ts,tsx}' -g '*theme*.{ts,tsx}' app/client/packages/design-system | xargs rg -A 2 'sizing-30|180px'

# Also check if there's any documentation about sizing in the design system
fd -g 'README.md' app/client/packages/design-system | xargs rg -i 'sizing|breakpoint'

Length of output: 129600

app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/metaConfig.ts (1)

1-14: LGTM! Clean and well-structured configuration.

The configuration follows a clear pattern and includes essential metadata for the DatePicker widget.

app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/helpers.ts (2)

1-2: LGTM!

Clean import statement with proper type import.


3-15: Verify date validation requirements

Need to confirm if additional date-specific validations are required (e.g., min/max date, format).

Let's check for any date validation requirements in the codebase:

✅ Verification successful

Additional date validations are already implemented

The codebase shows that date validation is properly handled:

  • Min/max date validation is implemented in derived.js through the isValid function
  • Date format validation is enforced through ValidationTypes.DATE_ISO_STRING
  • The current validation implementation correctly integrates with these existing validations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for date validation patterns in other widgets
rg -t typescript -t javascript "date.*valid|valid.*date" app/client/src/modules/ui-builder

# Check for date-related props in the widget types
ast-grep --pattern 'interface WDSDatePickerWidgetProps {
  $$$
}'

Length of output: 195


Script:

#!/bin/bash
# Search for date validation patterns with correct file type
rg -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" "date.*valid|valid.*date" app/client/src/modules/ui-builder

# Look for the widget props interface
rg -g "*.ts" -g "*.tsx" "interface.*DatePickerWidgetProps" -A 10 app/client/src/modules/ui-builder

# Search for date validation in the widget's directory
rg -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" "validation|valid|isValid" app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget

Length of output: 8321

app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/autocompleteConfig.ts (2)

4-6: Verify documentation URL validity.

The documentation URL should be verified to ensure it's accessible and points to the correct widget reference.

✅ Verification successful

Documentation URLs follow consistent pattern and are properly structured

All widget documentation URLs in the codebase follow the consistent pattern https://docs.appsmith.com/widget-reference/{widget-name}. The DatePicker widget's documentation URL matches this pattern.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the documentation URL pattern is consistent
# Expected: All widget docs should follow the same URL pattern

# Search for documentation URLs across widgets
rg -g '*.ts' '"!url":.*widget-reference' app/client/src/modules/ui-builder/

Length of output: 3821


1-1: Verify the import usage across the widget.

The import from WidgetUtils seems correct, but let's verify its usage across the widget implementation.

✅ Verification successful

Import usage is consistent with other WDS widgets

The DefaultAutocompleteDefinitions import is correctly used in the WDSDatePickerWidget, following the same pattern as other WDS widgets. All WDS widgets consistently:

  • Import DefaultAutocompleteDefinitions from "widgets/WidgetUtils"
  • Use it to define the isVisible property in their autocomplete configurations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check how DefaultAutocompleteDefinitions is used across the widget
# Expected: Consistent usage pattern across similar widgets

# Search for DefaultAutocompleteDefinitions usage
rg "DefaultAutocompleteDefinitions" app/client/src/modules/ui-builder/ui/wds/

# Check if other WDS widgets follow the same pattern
fd -e ts -e tsx . app/client/src/modules/ui-builder/ui/wds/ --exec grep -l "autocompleteConfig"

Length of output: 11620

app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/defaultsConfig.ts (2)

16-16: Consider adding validation for timePrecision

The timePrecision value should be validated against allowed values.


8-8: Verify empty string as defaultOptionValue

An empty string as defaultOptionValue might not be the most intuitive default. Consider using null or today's date as the default value.

✅ Verification successful

Let's search for more context about how this default value is used in the widget. This will help us understand if an empty string is appropriate or if we should suggest a different default value.


I need to check how this defaultOptionValue is actually used in the widget implementation to determine if an empty string is appropriate or if we should suggest a different default value.


I need to see the widget's implementation to understand how the defaultOptionValue is used in the actual date picker component before making a recommendation about its default value.


Empty string is appropriate for defaultOptionValue

Looking at the implementation, the empty string default is correct because:

  1. The widget properly handles undefined/empty values in the parseDate method
  2. The value is properly transformed in the derived properties for both selectedDate and formattedDate
  3. The component uses a separate defaultDate prop for actual default value handling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check how defaultOptionValue is used in the widget
ast-grep --pattern 'defaultOptionValue: $_' 

Length of output: 110940


Script:

#!/bin/bash
# Let's check how defaultOptionValue is used in the WDSDatePickerWidget
rg -A 5 -B 5 "defaultOptionValue" app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/

Length of output: 1284


Script:

#!/bin/bash
# Let's check the widget implementation to see how defaultOptionValue is used
rg -A 10 "defaultOptionValue" app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/ --type ts

Length of output: 1335


Script:

#!/bin/bash
# Let's check the widget component to see how the date picker is implemented
rg -l "class.*DatePicker" app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/ --type ts

# Then let's look at its content
ast-grep --pattern 'class $_ extends $_ {
  $$$
}'  app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/

Length of output: 13892

app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.js (2)

9-20: LGTM! Well-structured validation logic

The early return validation logic is clean and handles all edge cases appropriately.


34-34: LGTM! Appropriate default behavior

The default return value of true is correct when no validation constraints are specified.

app/client/src/modules/ui-builder/ui/wds/constants.ts (1)

64-64: LGTM! The widget mapping follows established conventions.

The addition of WDS_DATEPICKER_WIDGET maintains consistency with existing widget mapping patterns and is properly placed in the configuration object.

app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/contentConfig.ts (1)

71-85: LGTM!

The Label section is well-structured with appropriate validation and binding properties.

app/client/src/widgets/index.ts (3)

91-91: LGTM! Import follows established patterns.

The import statement follows the consistent pattern used for other WDS widgets.


189-189: LGTM! Widget correctly added to WDSWidgets array.

The widget is properly added to the WDSWidgets array and will be included in the final Widgets export.


91-91: Verify widget source and uniqueness

Let's ensure the widget is properly exported and there are no duplicate implementations.

Also applies to: 189-189

✅ Verification successful

Let me verify the export of the specific widget.


Widget is properly exported and integrated

The WDSDatePickerWidget is correctly exported from its source location and follows the expected module structure. While there are multiple date picker implementations in the codebase, this is a WDS-specific implementation that doesn't conflict with existing ones.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the widget export exists
rg -l "export.*WDSDatePickerWidget" "modules/ui-builder/ui/wds"

# Check for any potential duplicate date picker implementations
rg -l "DatePicker" --type ts

Length of output: 3989


Script:

#!/bin/bash
# Check the export in the WDSDatePickerWidget index file
cat "app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/index.ts"

# Check if there are any other exports of WDSDatePickerWidget
rg "export.*WDSDatePickerWidget" --type ts

Length of output: 698

app/client/src/widgets/WidgetUtils.ts (1)

996-1013: LGTM! Well-documented with clear examples

The JSDoc documentation is comprehensive and includes:

  • Clear example of input and output
  • Important usage rule about not destructuring props

* Main rule to remember is don't deconstruct the props like `const { value } = props;` in the derived property function.
* Directly access props like `props.value`
*/
export function parseDerivedProperties(propertyFns: Record<string, unknown>) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new piece of code. I need to use it for all widgets as well that I am doing here - #37390

@jsartisan
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12028958250.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37711.
recreate: .

@jsartisan jsartisan requested review from KelvinOm and znamenskii-ilia and removed request for jacquesikot November 26, 2024 15:15
@jsartisan jsartisan force-pushed the chore/wds-datepicker-widget branch from 93c8a9e to 7a21bc8 Compare November 28, 2024 05:41
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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)
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/contentConfig.ts (2)

22-33: Consider adding a default value for defaultDate

Adding a default value (like current date) would improve initial widget state and user experience.

 propertyName: "defaultDate",
 label: "Default Date",
 helpText:
   "Sets the default date of the widget. The date is updated if the default date changes",
 controlType: "DATE_PICKER",
 placeholderText: "Enter Default Date",
 useValidationMessage: true,
 isJSConvertible: true,
 isBindProperty: true,
 isTriggerProperty: false,
+defaultValue: new Date().toISOString(),
 validation: { type: ValidationTypes.DATE_ISO_STRING },

157-166: Improve loading animation documentation

The helpText could be more descriptive about what the loading animation looks like and when it appears.

-helpText: "Controls the loading of the widget",
+helpText: "Shows a skeleton loading state when fetching data or during updates",
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx (2)

2-2: Consider replacing 'moment' with a modern date library

The moment library is in maintenance mode. Consider using dayjs or date-fns for better performance and smaller bundle sizes.


57-59: Simplify derived properties using functions

Instead of using string interpolation for derived properties, define them as functions for better readability and maintainability.

Apply this refactor:

-static getDerivedPropertiesMap() {
-  const parsedDerivedProperties = parseDerivedProperties(derivedPropertyFns);
-
-  return {
-    isValid: `{{(() => {${parsedDerivedProperties.isValid}})()}}`,
-    selectedDate: `{{ this.value ? moment(this.value).toISOString() : "" }}`,
-    formattedDate: `{{ this.value ? moment(this.value).format(this.dateFormat) : "" }}`,
-  };
+static getDerivedPropertiesMap() {
+  return {
+    isValid: () => { /* implementation */ },
+    selectedDate: () => this.value ? moment(this.value).toISOString() : "",
+    formattedDate: () => this.value ? moment(this.value).format(this.dateFormat) : "",
+  };
 }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 93c8a9e and 7a21bc8.

📒 Files selected for processing (19)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/anvilConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/autocompleteConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/defaultsConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/index.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/metaConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/methodsConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/contentConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/index.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/settersConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/constants.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/index.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.js (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.test.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/helpers.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/types.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/constants.ts (1 hunks)
  • app/client/src/widgets/WidgetUtils.ts (1 hunks)
  • app/client/src/widgets/index.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/anvilConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/autocompleteConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/defaultsConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/index.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/metaConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/methodsConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/index.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/settersConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/constants.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/index.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.js
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.test.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/helpers.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/types.ts
  • app/client/src/modules/ui-builder/ui/wds/constants.ts
  • app/client/src/widgets/index.ts
🔇 Additional comments (8)
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/contentConfig.ts (4)

1-4: LGTM!

Clean imports and proper module structure.


71-85: LGTM!

Label configuration is properly structured with appropriate validation.


86-122: Add cross-field validation for date range

Consider adding validation to ensure minDate is not greater than maxDate when both are set.


173-180: Add event callback signature documentation

The helpText should include information about the callback parameters and their types.

app/client/src/widgets/WidgetUtils.ts (2)

996-1013: LGTM! Well-documented with clear examples

The JSDoc documentation is comprehensive and includes a helpful example along with important usage notes.


992-1036: 🛠️ Refactor suggestion

Add error handling and type safety improvements

The implementation needs defensive programming for robustness:

  1. Input validation for null/undefined propertyFns
  2. Error handling for malformed function strings
  3. Type safety for return value
-export function parseDerivedProperties(propertyFns: Record<string, unknown>) {
+export function parseDerivedProperties(propertyFns: Record<string, unknown>): Record<string, string> {
+  if (!propertyFns) {
+    return {};
+  }
+
   const derivedProperties: Record<string, string> = {};
+  const propsRegex = new RegExp(`props\\.`, "g");
 
   for (const [key, value] of Object.entries(propertyFns)) {
     if (typeof value === "function") {
-      const functionString = value.toString();
-      const functionBody = functionString.match(/(?<=\{)(.|\n)*(?=\})/)?.[0];
+      try {
+        const functionString = value.toString();
+        const functionBody = functionString.match(/(?<=\{)(.|\n)*(?=\})/)?.[0];
 
-      if (functionBody) {
-        const paramMatch = functionString.match(/\((.*?),/);
-        const propsParam = paramMatch ? paramMatch[1].trim() : "props";
+        if (functionBody) {
+          const paramMatch = functionString.match(/\((.*?),/);
+          const propsParam = paramMatch ? paramMatch[1].trim() : "props";
 
-        const modifiedBody = functionBody
-          .trim()
-          .replace(new RegExp(`${propsParam}\\.`, "g"), "this.");
+          const modifiedBody = functionBody
+            .trim()
+            .replace(propsRegex, "this.");
 
-        derivedProperties[key] = modifiedBody;
+          derivedProperties[key] = modifiedBody;
+        }
+      } catch (error) {
+        console.error(`Failed to parse derived property ${key}:`, error);
       }
     }
   }

Likely invalid or redundant comment.

app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx (2)

96-108: Ensure event triggers are properly configured

Verify that the onDateSelected event is correctly set up and that all necessary properties are being passed.


112-115: ⚠️ Potential issue

Correct the conditional logic in shouldResetDirtyState

The elif statement should be an else if in JavaScript/TypeScript. Also, double-check the logic to ensure that the isDirty state resets appropriately.

Apply this fix:

-const hasDefaultDateChanged = defaultDate !== prevProps.defaultDate;
-
-return hasDefaultDateChanged && isDirty;
+if (defaultDate !== prevProps.defaultDate && isDirty) {
+  return true;
+}
+return false;

Likely invalid or redundant comment.

@jsartisan
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12064380742.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37711.
recreate: .

Copy link

Deploy-Preview-URL: https://ce-37711.dp.appsmith.com

Copy link
Collaborator

@KelvinOm KelvinOm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ichik Should this widget morph to other input type widgets?

tags: [WIDGET_TAGS.INPUTS],
needsMeta: true,
searchTags: [
"date",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ichik Could you check this please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ichik Could you check this please?

@KelvinOm @jsartisan pushed an update to metaconfig.

KelvinOm
KelvinOm previously approved these changes Nov 28, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
app/client/packages/design-system/widgets/src/components/Calendar/src/styles.module.css (1)

64-69: LGTM! Consider extracting the box-shadow offset to theme variables.

The focus state implementation using double-layered box-shadow is a solid approach for accessibility.

Consider moving the --box-shadow-offset to your theme variables for consistent focus states across components:

- --box-shadow-offset: 2px;
+ --box-shadow-offset: var(--focus-ring-offset, 2px);
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7a21bc8 and 6be7d30.

📒 Files selected for processing (2)
  • app/client/packages/design-system/widgets/src/components/Calendar/src/styles.module.css (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx

@jsartisan jsartisan added the ok-to-test Required label for CI label Nov 28, 2024
@ichik
Copy link
Contributor

ichik commented Nov 28, 2024

@ichik Should this widget morph to other input type widgets?

@KelvinOm I would think so.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/metaConfig.ts (2)

3-18: Add JSDoc documentation for the configuration object

The configuration structure looks good, but adding JSDoc would improve maintainability and help other developers understand the purpose of needsMeta.

+/**
+ * Metadata configuration for the DatePicker widget
+ * @property {string} name - The display name of the widget
+ * @property {string[]} tags - Widget categorization tags
+ * @property {boolean} needsMeta - Indicates if the widget requires metadata processing
+ * @property {string[]} searchTags - Keywords for widget search functionality
+ */
 export const metaConfig = {

3-18: Consider adding TypeScript interface definition

Adding type definitions would improve type safety and provide better IDE support.

interface WidgetMetaConfig {
  name: string;
  tags: string[];
  needsMeta: boolean;
  searchTags: string[];
}

export const metaConfig: WidgetMetaConfig = {
  // ... existing configuration
};
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6be7d30 and da9454a.

📒 Files selected for processing (1)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/metaConfig.ts (1 hunks)

@jsartisan
Copy link
Contributor Author

@KelvinOm I have added the input morphing as well in this PR.

@jsartisan jsartisan force-pushed the chore/wds-datepicker-widget branch from da9454a to 2ab2d75 Compare November 29, 2024 06:42
@jsartisan jsartisan requested a review from KelvinOm November 29, 2024 06:43
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/contentConfig.ts (2)

43-76: Add validation message for timePrecision

The validation doesn't provide a user-friendly message when an invalid value is provided.

 validation: {
   type: ValidationTypes.TEXT,
   params: {
     allowedValues: ["day", "hour", "minute", "second"],
     default: "day",
+    errorMessage: "Time precision must be one of: day, hour, minute, second"
   },
 },

134-143: Add maxLength validation for tooltip

The tooltip text should have a reasonable length limit to prevent UI issues.

 validation: { 
-  type: ValidationTypes.TEXT 
+  type: ValidationTypes.TEXT,
+  params: {
+    maxLength: 150
+  }
 },
app/client/src/widgets/WidgetUtils.ts (2)

996-1013: Improve JSDoc documentation

The JSDoc example is helpful but could be improved by:

  1. Adding @param and @returns tags
  2. Documenting possible errors
  3. Adding more examples for edge cases

1017-1029: Optimize regex pattern creation

The RegExp creation inside the loop is inefficient. Move it outside the loop as shown in the previous diff.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between da9454a and 2ab2d75.

📒 Files selected for processing (22)
  • app/client/packages/design-system/widgets/src/components/Calendar/src/styles.module.css (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSBaseInputWidget/constants.ts (2 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/anvilConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/autocompleteConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/defaultsConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/index.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/metaConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/methodsConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/contentConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/index.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/settersConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/constants.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/index.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.js (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.test.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/helpers.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/types.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSInputWidget/config/propertyPaneConfig/contentConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/constants.ts (1 hunks)
  • app/client/src/widgets/WidgetUtils.ts (1 hunks)
  • app/client/src/widgets/index.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
  • app/client/packages/design-system/widgets/src/components/Calendar/src/styles.module.css
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/anvilConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/autocompleteConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/defaultsConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/index.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/metaConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/methodsConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/index.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/settersConfig.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/constants.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/index.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.js
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/derived.test.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/helpers.ts
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx
  • app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/types.ts
  • app/client/src/modules/ui-builder/ui/wds/constants.ts
  • app/client/src/widgets/index.ts
🔇 Additional comments (8)
app/client/src/modules/ui-builder/ui/wds/WDSBaseInputWidget/constants.ts (2)

9-9: LGTM! The DATE input type addition follows the established pattern.

The new DATE type is consistently defined with other input types in the constant.


20-20: LGTM! The widget mapping follows the naming convention.

The mapping of DATE to WDS_DATEPICKER_WIDGET is consistent with existing mappings and follows the established pattern.

Let's verify the widget type exists:

✅ Verification successful

Let me search for the widget type definition and its usage to verify if it exists and follows the naming convention.


Verified: The WDS_DATEPICKER_WIDGET type exists and follows the naming convention

The widget type is properly defined in the codebase and consistently used across multiple files:

  • Defined as a class type in WDSDatePickerWidget/widget/index.tsx
  • Referenced in the WDS constants
  • Correctly mapped to INPUT_TYPES.DATE in WDSBaseInputWidget/constants.ts
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the WDS_DATEPICKER_WIDGET type is properly defined
# Expected: Find the widget definition file and verify it exports the correct type

# Search for the widget definition
ast-grep --pattern 'export const WDS_DATEPICKER_WIDGET = $_'

Length of output: 141955


Script:

#!/bin/bash
# Search for WDS_DATEPICKER_WIDGET type definition
rg -l "WDS_DATEPICKER_WIDGET" --type ts

# Search for any references to this widget type
rg "WDS_DATEPICKER_WIDGET" --type ts -C 2

Length of output: 1653

app/client/src/modules/ui-builder/ui/wds/WDSInputWidget/config/propertyPaneConfig/contentConfig.ts (2)

51-54: LGTM on the date option addition.

The new date input type option is correctly structured and consistent with existing options.

Let's verify the integration with existing helper functions:


Line range hint 78-93: Consider updating defaultText validation for date type.

The defaultText validation should handle the new DATE type appropriately. Currently, it expects "string or number" as shown in the validation message.

Let's check the validation implementation:

app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/config/propertyPaneConfig/contentConfig.ts (3)

1-10: LGTM! Good code reuse

Clean imports and effective reuse of the input widget configuration.


94-130: Add cross-field validation for date range

The minDate and maxDate properties need cross-field validation to ensure minDate is not greater than maxDate.


181-188: Add event callback signature documentation

The helpText should include information about the callback parameters and their types.

app/client/src/widgets/WidgetUtils.ts (1)

992-1036: ⚠️ Potential issue

Add error handling and input validation

The function needs defensive programming for robustness:

  1. Input validation for null/undefined propertyFns
  2. Error handling for malformed function strings
  3. Type safety for return value

Apply this improvement:

-export function parseDerivedProperties(propertyFns: Record<string, unknown>) {
+export function parseDerivedProperties(propertyFns: Record<string, unknown>): Record<string, string> {
+  if (!propertyFns) {
+    return {};
+  }
+
   const derivedProperties: Record<string, string> = {};
+  const propsRegex = new RegExp(`props\\.`, "g");
 
   for (const [key, value] of Object.entries(propertyFns)) {
     if (typeof value === "function") {
-      const functionString = value.toString();
-      const functionBody = functionString.match(/(?<=\{)(.|\n)*(?=\})/)?.[0];
+      try {
+        const functionString = value.toString();
+        const functionBody = functionString.match(/(?<=\{)(.|\n)*(?=\})/)?.[0];
 
-      if (functionBody) {
-        const paramMatch = functionString.match(/\((.*?),/);
-        const propsParam = paramMatch ? paramMatch[1].trim() : "props";
+        if (functionBody) {
+          const paramMatch = functionString.match(/\((.*?),/);
+          const propsParam = paramMatch ? paramMatch[1].trim() : "props";
 
-        const modifiedBody = functionBody
-          .trim()
-          .replace(new RegExp(`${propsParam}\\.`, "g"), "this.");
+          const modifiedBody = functionBody
+            .trim()
+            .replace(propsRegex, "this.");
 
-        derivedProperties[key] = modifiedBody;
+          derivedProperties[key] = modifiedBody;
+        }
+      } catch (error) {
+        console.error(`Failed to parse derived property ${key}:`, error);
       }
     }
   }

Likely invalid or redundant comment.

@@ -6,6 +6,7 @@ export const INPUT_TYPES = {
PASSWORD: "PASSWORD",
PHONE_NUMBER: "PHONE_NUMBER",
MULTI_LINE_TEXT: "MULTI_LINE_TEXT",
DATE: "DATE",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the drop, date picker widget has an empty Data type. Could you please fix this?

Снимок экрана 2024-11-29 в 11 38 37

@jsartisan jsartisan merged commit 63eec76 into release Nov 29, 2024
49 of 50 checks passed
@jsartisan jsartisan deleted the chore/wds-datepicker-widget branch November 29, 2024 10:51
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Dec 2, 2024
![CleanShot 2024-11-26 at 15 56
15](https://github.com/user-attachments/assets/d812f475-11e1-4750-9018-bdd39d5a5de3)

/ok-to-test tags="@tag.Anvil"

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Release Notes

- **New Features**
- Introduced the WDS Date Picker Widget, enhancing date selection
capabilities within the UI.
- Added configuration options for widget size, visibility, and
autocomplete behavior.
- Implemented comprehensive validation for date input, ensuring accurate
user selections.
- Expanded widget collection to include the new WDS Date Picker Widget.
- Introduced new constants for date format options, facilitating diverse
formatting choices.
- Added support for a new "Date" input type, enhancing input widget
configurability.

- **Documentation**
- Updated property pane configurations to include detailed settings for
date format, validation, and event handling.

- **Bug Fixes**
- Improved handling of derived properties to ensure proper context
during widget interactions.

These updates collectively improve user experience and flexibility in
date selection within the application.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/12083004714>
> Commit: b17348e
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12083004714&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Anvil`
> Spec:
> <hr>Fri, 29 Nov 2024 10:44:39 UTC
<!-- end of auto-generated comment: Cypress test results  -->

---------

Co-authored-by: Vadim Vaitenko <vadim@appsmith.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants