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.

Sign up for GitHub

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: Datepicker Widget #36605

Closed
wants to merge 12 commits into from
Closed

chore: Datepicker Widget #36605

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2cf7b03
add datepicker component
Sep 26, 2024
f856211
add styles for focus
Sep 30, 2024
7f8c87a
update gap
Sep 30, 2024
333fd4d
remove unused code
Sep 30, 2024
321cc43
self code-review fixes
Sep 30, 2024
8f30f3a
minor typos
Sep 30, 2024
d2cde29
self code-review fixes
Sep 30, 2024
733f523
update validate helper
Sep 30, 2024
9443462
update parseDerivedProperties
Sep 30, 2024
63a5e91
update validate helper
Sep 30, 2024
2ce40e9
fix typo
Sep 30, 2024
4daa071
update disabled styles
Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/client/packages/design-system/widgets/src/components/Calender/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./src";
39 changes: 39 additions & 0 deletions app/client/packages/design-system/widgets/src/components/Calender/src/Calendar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import React from "react";
import type {
DateValue,
CalendarProps as HeadlessCalendarProps,
} from "react-aria-components";
import {
CalendarGrid as HeadlessCalendarGrid,
CalendarGridBody as HeadlessCalendarGridBody,
CalendarGridHeader as HeadlessCalendarGridHeader,
Calendar as HeadlessCalendar,
} from "react-aria-components";
import { Flex, IconButton } from "@appsmith/wds";

import styles from "./styles.module.css";
import { CalendarCell } from "./CalendarCell";
import { CalendarHeading } from "./CalendarHeading";
import { CalendarHeaderCell } from "./CalendarHeaderCell";

type CalendarProps<T extends DateValue> = HeadlessCalendarProps<T>;

export const Calendar = <T extends DateValue>(props: CalendarProps<T>) => {
return (
<HeadlessCalendar {...props} className={styles.calendar}>
<Flex alignItems="center" justifyContent="space-between" width="100%">
<IconButton icon="chevron-left" slot="previous" variant="ghost" />
<CalendarHeading size="subtitle" />
<IconButton icon="chevron-right" slot="next" variant="ghost" />
</Flex>
<HeadlessCalendarGrid>
<HeadlessCalendarGridHeader>
{(day) => <CalendarHeaderCell>{day}</CalendarHeaderCell>}
</HeadlessCalendarGridHeader>
<HeadlessCalendarGridBody>
{(date) => <CalendarCell date={date} />}
</HeadlessCalendarGridBody>
</HeadlessCalendarGrid>
</HeadlessCalendar>
);
};
18 changes: 18 additions & 0 deletions app/client/packages/design-system/widgets/src/components/Calender/src/CalendarCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import React from "react";
import { Text } from "@appsmith/wds";
import { CalendarCell as HeadlessCalendarCell } from "react-aria-components";

import styles from "./styles.module.css";
import type { CalendarCellProps } from "./types";

function CalendarCell(props: CalendarCellProps) {
const { date } = props;

return (
<HeadlessCalendarCell {...props} className={styles["calendar-cell"]}>
<Text>{date.day}</Text>
jsartisan marked this conversation as resolved.
Show resolved Hide resolved
</HeadlessCalendarCell>
);
}

export { CalendarCell };
19 changes: 19 additions & 0 deletions app/client/packages/design-system/widgets/src/components/Calender/src/CalendarHeaderCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import React from "react";
import { Text } from "@appsmith/wds";
import { CalendarHeaderCell as HeadlessCalendarHeaderCell } from "react-aria-components";

import type { CalendarHeaderCellProps } from "./types";

function CalendarHeaderCell(props: CalendarHeaderCellProps) {
const { children } = props;

return (
<HeadlessCalendarHeaderCell {...props}>
<Text color="neutral" fontWeight={700} textAlign="center">
{children}
</Text>
Comment on lines +12 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Let's discuss your styling choices, shall we?

I appreciate your use of the Text component for consistent styling. The neutral color, bold weight, and centered alignment are good choices for a header cell. However, remember that in design, as in education, flexibility is key!

Consider making these style properties customizable through props. This way, the component can adapt to different contexts, just like how we adjust our teaching methods for different students. Here's an example:

function CalendarHeaderCell({ 
  children, 
  color = "neutral", 
  fontWeight = 700, 
  textAlign = "center", 
  ...props 
}: CalendarHeaderCellProps & TextProps) {
  return (
    <HeadlessCalendarHeaderCell {...props}>
      <Text color={color} fontWeight={fontWeight} textAlign={textAlign}>
        {children}
      </Text>
    </HeadlessCalendarHeaderCell>
  );
}

This approach allows users of your component to customize the styling if needed, while still providing sensible defaults.

</HeadlessCalendarHeaderCell>
);
}

export { CalendarHeaderCell };
21 changes: 21 additions & 0 deletions app/client/packages/design-system/widgets/src/components/Calender/src/CalendarHeading.tsx
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need this component because calender consumes HeadingContext to display the months in popover header. so had to create this so that I could wrap the text in our Text component.

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { Text, type TextProps } from "@appsmith/wds";
import React, { forwardRef, type ForwardedRef } from "react";
import { HeadingContext, useContextProps } from "react-aria-components";

function CalendarHeading(
props: TextProps,
ref: ForwardedRef<HTMLHeadingElement>,
) {
[props, ref] = useContextProps(props, ref, HeadingContext);
const { children, ...domProps } = props;

return (
<Text {...domProps} color="neutral" ref={ref}>
{children}
</Text>
);
}

const _CalendarHeading = forwardRef(CalendarHeading);

export { _CalendarHeading as CalendarHeading };
4 changes: 4 additions & 0 deletions app/client/packages/design-system/widgets/src/components/Calender/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export { Calendar } from "./Calendar";
export { CalendarCell } from "./CalendarCell";
export { CalendarHeading } from "./CalendarHeading";
export { CalendarHeaderCell } from "./CalendarHeaderCell";
66 changes: 66 additions & 0 deletions app/client/packages/design-system/widgets/src/components/Calender/src/styles.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
.calendar {
padding: var(--outer-spacing-3);
}

.calendar table {
display: flex;
flex-direction: column;
margin: 0;
}

.calendar thead tr {
display: flex;
justify-content: space-around;
padding-block-start: var(--inner-spacing-1);
}

.calendar tbody tr {
display: flex;
justify-content: space-between;
}

.calendar thead th {
display: flex;
align-items: center;
justify-content: center;
inline-size: var(--sizing-9);
block-size: var(--sizing-9);
}

.calendar tbody td {
padding: var(--inner-spacing-1);
}

.calendar tbody [role="button"] {
display: flex;
align-items: center;
justify-content: center;
inline-size: var(--sizing-9);
block-size: var(--sizing-9);
border-radius: var(--border-radius-elevation-3);
border: var(--border-width-2) solid transparent;
text-align: center;
}

.calendar tbody [role="button"][data-disabled] {
opacity: var(--opacity-disabled);
}

.calendar tbody [role="button"][data-hovered] {
background-color: var(--color-bg-accent-subtle-hover);
cursor: pointer;
}

.calendar tbody [role="button"][data-pressed] {
background-color: var(--color-bg-accent-subtle-active);
}

.calendar tbody [role="button"][data-selected] {
background-color: var(--color-bg-accent);
color: var(--color-fg-on-accent);
}

.calendar tbody [role="button"][data-focus-visible],
.calendar tbody [role="button"][data-focused] {
border-color: var(--color-bd-accent);
}
10 changes: 10 additions & 0 deletions app/client/packages/design-system/widgets/src/components/Calender/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import {
type CalendarCellProps as HeadlessCalendarCellProps,
type CalendarHeaderCellProps as HeadlessCalendarHeaderCellProps,
} from "react-aria-components";

export type CalendarCellProps = HeadlessCalendarCellProps &
React.RefAttributes<HTMLTableCellElement>;

export type CalendarHeaderCellProps = HeadlessCalendarHeaderCellProps &
React.RefAttributes<HTMLTableCellElement>;
12 changes: 12 additions & 0 deletions app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,18 @@
padding-inline: var(--inner-spacing-2);
}

.input:is([data-date-input]) {
display: flex;
gap: calc(var(--inner-spacing-1) / 2);
align-items: center;
}

.input:is([data-date-input]) [data-focused="true"] {
background-color: var(--color-bg-accent);
color: var(--color-fg-on-accent);
box-shadow: 0 0 0 1px var(--color-bd-focus);
}

.inputWrapper:has([data-hovered]) {
background-color: var(--color-bg-neutral-subtle-hover);
box-shadow: inset 0 0 0 1px var(--color-bd-on-neutral-subtle-hover);
Expand Down
1 change: 1 addition & 0 deletions app/client/packages/design-system/widgets/src/components/DatePicker/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./src";
70 changes: 70 additions & 0 deletions app/client/packages/design-system/widgets/src/components/DatePicker/src/DatePicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import {
FieldError,
FieldDescription,
FieldLabel,
FieldCalenderPopover,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the spelling of 'Calendar' in 'FieldCalenderPopover'

Attention to detail is important. There is a typo in your import statement. The component 'FieldCalenderPopover' should be spelled 'FieldCalendarPopover'. Let's correct this to ensure the component is properly imported and to avoid any runtime errors.

Apply this diff to fix the typo:

 import {
   FieldError,
   FieldDescription,
   FieldLabel,
-  FieldCalenderPopover,
+  FieldCalendarPopover,
   Button,
 } from "@appsmith/wds";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
FieldCalenderPopover,
FieldCalendarPopover,

Button,
} from "@appsmith/wds";
import { getTypographyClassName } from "@appsmith/wds-theming";
import clsx from "clsx";
import React from "react";
import {
DateInput,
DateSegment,
Group,
DatePicker as HeadlessDatePicker,
} from "react-aria-components";

import type { DatePickerProps } from "./types";
import styles from "../../ComboBox/src/styles.module.css";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just reusing the styels from combobox.

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Today's lesson: Component Independence

Now, let's talk about component design. Who can tell me what's unusual about this line?

import styles from "../../ComboBox/src/styles.module.css";

That's correct, Sarah! We're importing styles from a different component. Can anyone tell me why this might not be the best practice?

Excellent point, Michael! It creates a dependency between components that should ideally be independent. For your homework, I want you all to think about how we could create a separate styles file for our DatePicker. This way, if we ever need to change the ComboBox styles, we won't accidentally affect our DatePicker!

Remember, class: in component design, independence is key!


export const DatePicker = (props: DatePickerProps) => {
const {
contextualHelp,
description,
errorMessage,
isLoading,
isRequired,
label,
size = "medium",
...rest
} = props;

return (
<HeadlessDatePicker
aria-label={Boolean(label) ? undefined : "DatePicker"}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Pop quiz: Boolean logic!

Alright, class, it's time for a pop quiz on Boolean logic! Can anyone spot the unnecessary operations in these lines?

aria-label={Boolean(label) ? undefined : "DatePicker"}
color={Boolean(isLoading) ? "neutral" : "accent"}
variant={Boolean(isLoading) ? "ghost" : "filled"}

Excellent observation, Emily! The Boolean() calls are redundant here. In JavaScript, values are automatically coerced to booleans in conditional statements. Let's clean up our code:

- aria-label={Boolean(label) ? undefined : "DatePicker"}
+ aria-label={label ? undefined : "DatePicker"}
- color={Boolean(isLoading) ? "neutral" : "accent"}
+ color={isLoading ? "neutral" : "accent"}
- variant={Boolean(isLoading) ? "ghost" : "filled"}
+ variant={isLoading ? "ghost" : "filled"}

Remember, class: simpler code is often clearer code. Always look for opportunities to remove redundant operations!

Also applies to: 58-58, 62-62

🧰 Tools
🪛 Biome

[error] 36-36: Avoid redundant Boolean call

It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call

(lint/complexity/noExtraBooleanCast)

className={styles.formField}
data-size={size}
isRequired={isRequired}
{...rest}
>
{({ isInvalid }) => (
<>
<FieldLabel
contextualHelp={contextualHelp}
isRequired={isRequired}
text={label}
/>
<Group className={styles.inputWrapper}>
<DateInput
className={clsx(styles.input, getTypographyClassName("body"))}
data-date-input
>
{(segment) => <DateSegment segment={segment} />}
</DateInput>
<Button
color={Boolean(isLoading) ? "neutral" : "accent"}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Simplify the conditional by removing unnecessary Boolean call

Remember, simplicity is key in coding. The Boolean(isLoading) call is unnecessary since isLoading will be coerced to a boolean in the conditional. Let's make the code cleaner by removing the Boolean call.

Apply this diff to simplify the code:

               color={Boolean(isLoading) ? "neutral" : "accent"}
+              color={isLoading ? "neutral" : "accent"}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
color={Boolean(isLoading) ? "neutral" : "accent"}
color={isLoading ? "neutral" : "accent"}
🧰 Tools
🪛 Biome

[error] 56-56: Avoid redundant Boolean call

It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call

(lint/complexity/noExtraBooleanCast)

icon="calendar-month"
isLoading={isLoading}
size={size === "medium" ? "small" : "xSmall"}
variant={Boolean(isLoading) ? "ghost" : "filled"}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Simplify the conditional by removing unnecessary Boolean call

To enhance code clarity, we should avoid redundant type casting. In this case, Boolean(isLoading) is not needed. Let's streamline the code by omitting the Boolean call.

Apply this diff to simplify the code:

               variant={Boolean(isLoading) ? "ghost" : "filled"}
+              variant={isLoading ? "ghost" : "filled"}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
variant={Boolean(isLoading) ? "ghost" : "filled"}
variant={isLoading ? "ghost" : "filled"}
🧰 Tools
🪛 Biome

[error] 60-60: Avoid redundant Boolean call

It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call

(lint/complexity/noExtraBooleanCast)

/>
Comment on lines +57 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Time for a Boolean logic pop quiz!

Alright, class, let's examine this Button component. Can anyone spot the redundant operations here? Yes, you in the back! Exactly right, we have two unnecessary Boolean() calls. Let's clean these up:

 <Button
-  color={Boolean(isLoading) ? "neutral" : "accent"}
+  color={isLoading ? "neutral" : "accent"}
   icon="calendar-month"
   isLoading={isLoading}
   size={size === "medium" ? "small" : "xSmall"}
-  variant={Boolean(isLoading) ? "ghost" : "filled"}
+  variant={isLoading ? "ghost" : "filled"}
 />

Excellent work! Remember, in conditional statements, variables are automatically coerced to booleans. We don't need to explicitly convert them. This makes our code cleaner and more efficient. Keep an eye out for these opportunities to simplify!

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Button
color={Boolean(isLoading) ? "neutral" : "accent"}
icon="calendar-month"
isLoading={isLoading}
size={size === "medium" ? "small" : "xSmall"}
variant={Boolean(isLoading) ? "ghost" : "filled"}
/>
<Button
color={isLoading ? "neutral" : "accent"}
icon="calendar-month"
isLoading={isLoading}
size={size === "medium" ? "small" : "xSmall"}
variant={isLoading ? "ghost" : "filled"}
/>
🧰 Tools
🪛 Biome

[error] 56-56: Avoid redundant Boolean call

It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call

(lint/complexity/noExtraBooleanCast)

[error] 60-60: Avoid redundant Boolean call

It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call

(lint/complexity/noExtraBooleanCast)

</Group>
<FieldError errorMessage={errorMessage} />
<FieldDescription description={description} isInvalid={isInvalid} />
<FieldCalenderPopover />
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Oops! We've made the same spelling mistake twice!

Class, do you remember the spelling error we corrected earlier in our import statement? Well, it looks like we've made the same mistake here in using the component. This is a great lesson in why we should use consistent naming throughout our code.

Let's correct this line:

- <FieldCalenderPopover />
+ <FieldCalendarPopover />

Now, can anyone tell me why it's important to be consistent with our naming? That's right! It helps prevent errors and makes our code easier to understand and maintain. Always double-check your spelling, especially when using custom components!

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<FieldCalenderPopover />
<FieldCalendarPopover />

</>
)}
</HeadlessDatePicker>
);
};
2 changes: 2 additions & 0 deletions app/client/packages/design-system/widgets/src/components/DatePicker/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { DatePicker } from "./DatePicker";
export type { DateValue } from "./types";
29 changes: 29 additions & 0 deletions app/client/packages/design-system/widgets/src/components/DatePicker/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import type {
DateValue,
DatePickerProps as SpectrumDatePickerProps,
ValidationResult,
} from "react-aria-components";
import type { SIZES } from "@appsmith/wds";

export interface DatePickerProps
extends Omit<SpectrumDatePickerProps<DateValue>, "slot" | "placeholder"> {
/** The content to display as the label. */
label?: string;
/** The content to display as the description. */
description?: string;
/** The content to display as the error message. */
errorMessage?: string | ((validation: ValidationResult) => string);
/** size of the select
*
* @default medium
*/
size?: Omit<keyof typeof SIZES, "xSmall" | "large">;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Let's improve our 'size' property type, shall we?

While your current implementation works, we can make it even better! Consider creating a separate type for the size property. This will make our code more readable and easier to maintain.

Here's how we could refactor this:

type DatePickerSize = Omit<keyof typeof SIZES, "xSmall" | "large">;

export interface DatePickerProps
  extends Omit<SpectrumDatePickerProps<DateValue>, "slot" | "placeholder"> {
  // ... other properties ...
  size?: DatePickerSize;
  // ... rest of the properties ...
}

This way, if we need to use this size type elsewhere, we can easily reuse it!

/** loading state for the input */
isLoading?: boolean;
/** A ContextualHelp element to place next to the label. */
contextualHelp?: string;
/** The content to display as the placeholder. */
placeholder?: string;
}

export type { DateValue };
Loading
Loading