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: Allow currency and phone dial code change #38141

Merged
merged 19 commits into from
Dec 19, 2024
Merged

Conversation

jsartisan
Copy link
Contributor

@jsartisan jsartisan commented Dec 13, 2024

CleanShot 2024-12-16 at 16 42 37

CleanShot 2024-12-16 at 16 43 12

Fixes #38091

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

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Added new props for enhanced styling and functionality in various components, including className and maxHeight.
    • Introduced dropdown menus for currency and ISD code selection in the CurrencyInput and PhoneInput components.
    • New story added for the TextField component showcasing prefix and suffix usage.
    • Added a custom hook useRootContainer for better theme provider management.
  • Bug Fixes

    • Improved conditional rendering for input elements based on prop values.
  • Style

    • Significant updates to CSS for input and select components, enhancing layout and visual feedback.
    • New CSS classes added for ComboBox and styling adjustments for existing classes.
    • Enhanced styling for the ComboBox popover and other components.
    • New CSS rules for currency and dial code options in respective widgets.
  • Documentation

    • Updated property configurations for widgets to include new toggle options for currency and dial code changes.

Tip

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


Wed, 18 Dec 2024 11:33:31 UTC

Copy link
Contributor

coderabbitai bot commented Dec 13, 2024

Walkthrough

This pull request introduces comprehensive updates across multiple design system and widget components, focusing on enhancing styling flexibility, interaction, and component functionality. The changes span input components, menu systems, currency and phone input widgets, and styling modules, with a consistent theme of improving user experience through more dynamic and customizable UI elements.

Changes

File Path Change Summary
app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx Added className prop, introduced localRef, updated prefix/suffix rendering logic
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css Refined .inputGroup and .input class styles, enhanced state handling
app/client/packages/design-system/widgets/src/components/Select/src/SelectTrigger.tsx Updated styling with clsx, modified suffix rendering
app/client/packages/design-system/widgets/src/components/Select/src/styles.module.css Added .selectInputGroup and .selectTriggerButton classes
app/client/packages/design-system/widgets/src/components/TextField/stories/TextField.stories.tsx Added WithPrefixAndSuffix story
app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/index.tsx Introduced currency selection dropdown
app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/index.tsx Added ISD code selection dropdown
app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBox.tsx Updated to use useRootContainer, enhanced Popover props
app/client/packages/design-system/widgets/src/components/Datepicker/src/Datepicker.tsx Integrated useRootContainer for root element management
app/client/packages/design-system/widgets/src/components/Popover/src/constants.ts Added POPOVER_LIST_BOX_MAX_HEIGHT constant
app/client/packages/design-system/widgets/src/components/Popover/src/index.ts Enhanced exports in index.ts
app/client/packages/design-system/widgets/src/components/Popover/src/useRootContainer.ts Introduced useRootContainer hook
app/client/packages/design-system/widgets/src/components/Select/src/Select.tsx Updated to use useRootContainer, added maxHeight prop to Popover
app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx Enhanced MenuProps to include className and maxHeight
app/client/packages/design-system/widgets/src/components/Menu/src/types.ts Updated MenuProps interface to omit additional properties
app/client/packages/design-system/widgets/src/components/MenuItem/src/MenuItem.tsx Added className prop to MenuItem component
app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/styles.module.css Added CSS rule for currency options
app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/styles.module.css Added CSS rule for dial code options
app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/config/propertyPaneConfig/contentConfig.ts Added allowDialCodeChange property configuration

Possibly Related PRs

Suggested Labels

Enhancement, Widgets Product, UI Improvement, Widgets & Accelerators Pod

Suggested Reviewers

  • ankitakinger
  • sagar-qa007

Poem

🎨 Code transforms, widgets dance and sway,
Inputs sparkle with newfound display,
Dropdowns bloom like digital flowers,
Flexibility reigns through coding powers!
A symphony of pixels, smooth and bright 🌟


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 508d3f8 and b8bb738.

📒 Files selected for processing (1)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css

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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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 Dec 13, 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: 2

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

1-14: LGTM! Consider using CSS custom property for flex alignment

The flexbox layout implementation looks good and properly handles the spacing between text and icon. For consistency with the design system, consider using CSS custom properties for the flex properties if they exist.

.selectTriggerButton {
-  display: flex;
-  align-items: center;
+  display: var(--flex-display, flex);
+  align-items: var(--flex-align-center, center);
}
app/client/packages/design-system/widgets/src/components/ListBox/src/styles.module.css (1)

7-9: LGTM! Consider adding browser compatibility comment.

The CSS calculation for min-inline-size looks correct and should fix the sizing issues. The use of :is() selector is elegant.

Consider adding a comment about :is() browser compatibility (IE11 doesn't support it) if legacy browser support is needed.

app/client/packages/design-system/widgets/src/components/Popover/src/styles.module.css (1)

8-10: Consider RTL support in transform.

The transform looks good for LTR layouts. However, for complete internationalization support:

Consider using logical properties with this pattern:

.popover:is([data-select-popover], [data-combobox-popover]) {
-  transform: translateX(calc(-1 * var(--inner-spacing-2, 0)));
+  transform: translate(calc(-1 * var(--inner-spacing-2, 0)), 0);
+  [dir="rtl"] & {
+    transform: translate(var(--inner-spacing-2, 0), 0);
+  }
}
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (1)

Line range hint 9-22: Combine duplicate .inputGroup definitions

The .inputGroup class is defined twice. Consider merging these definitions to improve maintainability.

app/client/packages/design-system/widgets/src/components/TextField/stories/TextField.stories.tsx (1)

47-47: Simplify the prefix prop

You can pass the string directly without wrapping it in a fragment.

- <TextField prefix={<>+121212</>} />
+ <TextField prefix="+121212" />
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6eb44a1 and 8274bc3.

📒 Files selected for processing (11)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBox.tsx (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (7 hunks)
  • app/client/packages/design-system/widgets/src/components/ListBox/src/styles.module.css (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Popover/src/styles.module.css (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Select/src/Select.tsx (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Select/src/SelectTrigger.tsx (3 hunks)
  • app/client/packages/design-system/widgets/src/components/Select/src/styles.module.css (1 hunks)
  • app/client/packages/design-system/widgets/src/components/TextField/stories/TextField.stories.tsx (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSSwitchWidget/config/settersConfig.ts (0 hunks)
💤 Files with no reviewable changes (1)
  • app/client/src/modules/ui-builder/ui/wds/WDSSwitchWidget/config/settersConfig.ts
✅ Files skipped from review due to trivial changes (1)
  • app/client/packages/design-system/widgets/src/components/Select/src/Select.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/Select/src/SelectTrigger.tsx

[error] 38-38: 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)

🔇 Additional comments (5)
app/client/packages/design-system/widgets/src/components/Select/src/SelectTrigger.tsx (1)

Line range hint 24-36: LGTM! Clean component structure with improved testability

The component structure looks good with proper data attributes for testing and styling hooks.

🧰 Tools
🪛 Biome (1.9.4)

[error] 25-25: 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)

app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBox.tsx (1)

54-57: Verify portal container query selector reliability.

The changes look good and improve accessibility. However, let's verify the reliability of the portal container selector.

✅ Verification successful

Portal container selector is reliable and consistently used

The data-theme-provider selector is reliably implemented across the design system:

  • Single instance per app, defined in ThemeProvider.tsx
  • Consistently queried using document.body.querySelector("[data-theme-provider]") or closest("[data-theme-provider]")
  • Used uniformly across all portal-based components (ComboBox, Sheet, Select, Menu, etc.)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential multiple data-theme-provider elements that could cause issues
rg -l 'data-theme-provider' | xargs -I {} sh -c 'echo "=== {} ==="; cat "{}" | grep -C 5 "data-theme-provider"'

Length of output: 4451

app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx (1)

176-179: LGTM! Addition of id prop improves item identification.

The change correctly adds the id prop to ListBoxItem while maintaining consistency with the key prop.

Let's verify if this change resolves the select widget bug:

✅ Verification successful

Addition of id prop to ListBoxItem is verified and correct

The change is properly implemented and consistent with React best practices:

  • The id prop is derived from option.id and matches the key prop
  • The implementation is the only occurrence of ListBoxItem in the codebase
  • The change aligns with the select widget bug fix objective
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any related test files or similar implementations
fd -e tsx -e ts "Select(Widget)?\.test\." -x cat {} \; 

Length of output: 76470


Script:

#!/bin/bash
# Let's check for any related ListBoxItem implementations and usages
ast-grep --pattern 'ListBoxItem $$$'

# Also check for any related test files specifically for WDSSelectWidget
fd -e tsx -e ts "WDSSelect(Widget)?\.test\." -x cat {} \;

Length of output: 101


Script:

#!/bin/bash
# Let's try a different approach to find ListBoxItem usage
rg -U "ListBoxItem.*id.*key" -A 2 -B 2

# Also search for any related select widget files
fd "WDSSelectWidget" -x ls -l {} \;

Length of output: 826

app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (1)

71-79: Verify :has() pseudo-class compatibility

The use of the :has() pseudo-class may not be fully supported in all target browsers. Ensure it aligns with the project's browser support requirements.

Also applies to: 127-128, 138-145, 155-157, 167-174

app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx (1)

49-49: Change looks good

Comment on lines 37 to 39
<span data-input-suffix>
{Boolean(isLoading) ? <Spinner /> : <Icon name="chevron-down" />}
</span>
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

Remove redundant Boolean conversion

The Boolean conversion is unnecessary as the condition will already be coerced to a boolean in the ternary operation.

<span data-input-suffix>
-  {Boolean(isLoading) ? <Spinner /> : <Icon name="chevron-down" />}
+  {isLoading ? <Spinner /> : <Icon name="chevron-down" />}
</span>
📝 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
<span data-input-suffix>
{Boolean(isLoading) ? <Spinner /> : <Icon name="chevron-down" />}
</span>
<span data-input-suffix>
{isLoading ? <Spinner /> : <Icon name="chevron-down" />}
</span>
🧰 Tools
🪛 Biome (1.9.4)

[error] 38-38: 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)

align-items: center;
box-sizing: content-box;
.inputGroup {
background-color: var(--color-bg);
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

Remove redundant background-color property

The background-color is set twice in the .inputGroup styles. Remove the redundant declaration to avoid confusion.

.inputGroup {
  /* ... */
- background-color: var(--color-bg);
  /* ... */
  background-color: var(--color-bg-neutral-subtle);
+ /* Removed duplicate background-color */
  /* ... */
}

Also applies to: 17-17

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: 1

🧹 Nitpick comments (4)
app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBoxTrigger.tsx (1)

Line range hint 22-26: Consider adding type safety for size values

While the size conditional logic is good, consider using a union type for the size prop to ensure type safety.

interface ComboBoxTriggerProps {
-  size?: ComboBoxProps["size"];
+  size?: "small" | "medium" | "large";
  isLoading?: boolean;
  isDisabled?: boolean;
  placeholder?: string;
}
app/client/packages/design-system/widgets/src/components/Input/src/types.ts (1)

18-20: LGTM! Consider adding JSDoc.

The new inputGroupClassName prop follows existing patterns. Consider adding JSDoc to document its purpose and usage.

app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (2)

72-82: Consider simplifying complex selectors

The nested :has() and :is() selectors might impact maintainability and browser support. Consider breaking this into multiple, simpler selectors.


128-132: Add comment explaining the complex selector

Consider documenting why both direct disabled inputs and nested disabled inputs need to be handled.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8274bc3 and 4ccdbf4.

📒 Files selected for processing (7)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBoxTrigger.tsx (3 hunks)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx (2 hunks)
  • app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (7 hunks)
  • app/client/packages/design-system/widgets/src/components/Input/src/types.ts (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Select/src/SelectTrigger.tsx (3 hunks)
  • app/client/packages/design-system/widgets/src/components/Select/src/styles.module.css (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx
  • app/client/packages/design-system/widgets/src/components/Select/src/styles.module.css
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/Select/src/SelectTrigger.tsx

[error] 40-40: 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)

🔇 Additional comments (10)
app/client/packages/design-system/widgets/src/components/Select/src/SelectTrigger.tsx (4)

1-7: LGTM! Clean import organization

The imports are well-structured and all dependencies are necessary for the component's functionality.


22-24: LGTM! Proper style composition

Good use of clsx for combining styles from different modules while maintaining separation of concerns.


26-26: LGTM! Consistent styling approach

The Button styling follows the same clean pattern as the Group component.


39-45: Remove redundant Boolean conversion

The Boolean conversion is unnecessary as the condition will already be coerced to a boolean in the ternary operation.

<span data-input-suffix>
-  {Boolean(isLoading) ? <Spinner /> : <Icon name="chevron-down" size="medium" />}
+  {isLoading ? <Spinner /> : <Icon name="chevron-down" size="medium" />}
</span>
🧰 Tools
🪛 Biome (1.9.4)

[error] 40-40: 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)

app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBoxTrigger.tsx (2)

2-2: LGTM: Clean import structure

The transition to CSS modules and direct component imports follows best practices.

Also applies to: 4-4


32-33: Verify CSS module classes exist

The styling approach looks good, but let's ensure the CSS classes are properly defined.

✅ Verification successful

CSS classes are properly defined and used correctly

Both comboboxInput and comboboxInputGroup classes exist in the styles module with appropriate padding styles. The implementation is valid.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the CSS classes exist in the styles module
rg -l "comboboxInput|comboboxInputGroup" "app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css"

Length of output: 218


Script:

#!/bin/bash
# Check the actual content of the CSS classes in the styles module
rg "comboboxInput|comboboxInputGroup" "app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css" -A 2

Length of output: 249

app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (4)

Line range hint 9-17: Remove redundant background-color property

The background-color is set twice in the .inputGroup styles.


24-29: LGTM! Clean separation of concerns.

Good refactor moving container styles to inputGroup.


139-149: LGTM! Consistent use of design system tokens.

Good use of negative color variables for error states.


168-180: LGTM! Consistent sizing implementation.

Good use of CSS calc() for maintaining consistent proportions across size variants.

@jsartisan jsartisan changed the title chore: fix select widget bug chore: Allow currency and phone dial code change Dec 16, 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: 6

🧹 Nitpick comments (4)
app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/index.tsx (2)

34-39: Add accessibility label to the button

Include an aria-label for the Button to improve accessibility.

<Button
  color="neutral"
  icon="chevron-down"
  iconPosition="end"
  size="small"
  variant="subtle"
+ aria-label="Select country code"
>

73-74: Use css prop instead of style

Replace the style prop with css for consistency with styled components.

- <Text style={{ whiteSpace: "nowrap" }}>{selectedCountry?.dial_code}</Text>
+ <Text css={{ whiteSpace: "nowrap" }}>{selectedCountry?.dial_code}</Text>
app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx (1)

22-26: Consider adding a default maxHeight value

While the maxHeight prop addition is good, consider adding a default value to ensure consistent behavior when the prop isn't provided.

- maxHeight={maxHeight}
+ maxHeight={maxHeight ?? 300} // or any other appropriate default value
app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/config/propertyPaneConfig/contentConfig.ts (1)

68-68: Update 'helpText' for clarity

The helpText for allowCurrencyChange currently says "Search by currency or country", which may not accurately describe the property. Consider changing it to "Allow users to change the currency" for better clarity.

{
  propertyName: "allowCurrencyChange",
  label: "Allow currency change",
- helpText: "Search by currency or country",
+ helpText: "Allow users to change the currency",
  controlType: "SWITCH",
  isJSConvertible: true,
  isBindProperty: true,
  isTriggerProperty: false,
  validation: { type: ValidationTypes.BOOLEAN },
},
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4ccdbf4 and 1644d08.

📒 Files selected for processing (13)
  • app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx (2 hunks)
  • app/client/packages/design-system/widgets/src/components/Menu/src/styles.module.css (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Menu/src/types.ts (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Menu/stories/Menu.stories.tsx (1 hunks)
  • app/client/packages/design-system/widgets/src/components/MenuItem/src/MenuItem.tsx (1 hunks)
  • app/client/packages/design-system/widgets/src/components/TextField/stories/TextField.stories.tsx (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/index.tsx (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/styles.module.css (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/utilities.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/config/propertyPaneConfig/contentConfig.ts (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/index.tsx (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/styles.module.css (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/config/propertyPaneConfig/contentConfig.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/styles.module.css
  • app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/styles.module.css
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/packages/design-system/widgets/src/components/TextField/stories/TextField.stories.tsx
🔇 Additional comments (5)
app/client/packages/design-system/widgets/src/components/Menu/src/types.ts (1)

3-5: Verify selection behavior compatibility

The removal of "selectionMode" and "selectedKeys" from Omit means these props are now available. Ensure this change is intentional and compatible with existing menu implementations.

✅ Verification successful

Let me analyze the actual usage of selection-related props in Menu components to verify compatibility.


Let me check the actual usage patterns of the Menu component to verify if selection-related props are being used.


Selection props removal is safe to proceed

Based on the codebase analysis, the removal of "selectionMode" and "selectedKeys" from Omit is safe as there are no direct usages of these props in Menu components. The existing Menu implementations focus on basic menu functionality without selection features.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing menu selection usage
rg -l "selectionMode|selectedKeys.*Menu" 

Length of output: 471


Script:

#!/bin/bash
# Check the actual usage of Menu components with selection-related props
rg "selectionMode.*Menu|selectedKeys.*Menu" -A 3

# Check Menu component implementations
ast-grep --pattern 'interface $_Props extends AriaMenuProps<$_> {
  $$$
}'

Length of output: 127


Script:

#!/bin/bash
# Search for Menu component usage patterns
rg "<Menu.*>" -A 2

# Search for Menu-related test files
fd "Menu.*test|Menu.*spec"

# Check for Menu-related component files
fd "Menu" --type f --exec grep -l "selectionMode\|selectedKeys" {}

Length of output: 59967

app/client/packages/design-system/widgets/src/components/MenuItem/src/MenuItem.tsx (1)

12-12: LGTM! Clean implementation of className support

The addition of className prop with clsx composition follows best practices for component customization.

Also applies to: 15-18

app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx (1)

5-5: LGTM: maxHeight prop addition for scroll control

The addition of maxHeight prop provides better control over menu dimensions, which is essential for handling long lists.

Also applies to: 11-11

app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/index.tsx (1)

31-77: Currency selection implementation looks good

The conditional rendering for currency selection in the prefix variable is well-implemented and aligns with the required functionality.

app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/utilities.ts (1)

67-75: Verify 'countryToFlag' function compatibility

The countryToFlag function uses String.fromCodePoint, which may not be supported in all environments. Ensure that this function works as expected across all target platforms or consider adding a polyfill if necessary.

Comment on lines 14 to 16
.menuPopover {
overflow-y: auto;
}
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

Review nested scrolling behavior

Both .menu and .menuPopover have overflow-y: auto. This could lead to nested scrolling containers, which might cause usability issues.

Consider consolidating the overflow behavior in one container:

.menuPopover {
-  overflow-y: auto;
+  /* Add other necessary styles without overflow */
}

Committable suggestion skipped: line range outside the PR's diff.

@github-actions github-actions bot added Anvil Pod Issue related to Anvil project Task A simple Todo labels Dec 16, 2024
@@ -0,0 +1,11 @@
div.comboboxInputGroup {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these styles are needed so that the popover takes full width.

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: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1644d08 and 50cd6dc.

📒 Files selected for processing (2)
  • app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx (2 hunks)
  • app/client/packages/design-system/widgets/src/components/Popover/src/styles.module.css (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx

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

🧹 Nitpick comments (2)
app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx (2)

54-58: Optimize Boolean conditions

The Boolean wrapper is redundant here as the value will already be coerced to boolean.

-      {Boolean(prefix) && (
+      {prefix && (

73-77: Optimize Boolean conditions

Similar to the prefix, the Boolean wrapper is redundant here.

-      {Boolean(suffix) && (
+      {suffix && (
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50cd6dc and 1987d37.

📒 Files selected for processing (3)
  • app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx (4 hunks)
  • app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (7 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx

[error] 66-66: 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] 67-67: 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)

🔇 Additional comments (3)
app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx (2)

Line range hint 13-26: LGTM: Proper ref handling and prop management

The implementation of ref merging and new props is clean and follows React best practices.


53-53: LGTM: Clean className composition

The use of clsx for merging classNames is appropriate and follows best practices.

app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx (1)

176-176: LGTM: Proper accessibility implementation

The addition of the id prop alongside the key prop improves accessibility while maintaining proper React list rendering.

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: 1

♻️ Duplicate comments (1)
app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/index.tsx (1)

23-27: ⚠️ Potential issue

Fix the key mismatch in country selection logic

The key prefix menu-item- in MenuItem will prevent the country lookup from working correctly in onMenuItemSelect.

Apply this fix:

- key={`menu-item-${item.code}`}
+ key={item.code}

Also applies to: 58-58

🧹 Nitpick comments (2)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (2)

73-81: Consider simplifying the hover state selector

The current selector is complex and might be hard to maintain. Consider breaking it down into separate state classes.

-.inputGroup[data-hovered]:has(
-    > .input:not(
-        :is(
-            [data-focused],
-            [data-readonly],
-            [data-disabled],
-            [data-focus-within],
-            :has(~ input[data-disabled="true"])
-          )
-      )
-  )
+.inputGroup[data-hovered]:not([data-focused]):not([data-readonly]):not([data-disabled])

169-181: Consider adding a medium size variant

The size variants are well implemented, but consider adding a medium size for completeness in the design system.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1987d37 and f4cf381.

📒 Files selected for processing (5)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (7 hunks)
  • app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx (2 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/index.tsx (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/config/propertyPaneConfig/contentConfig.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/config/propertyPaneConfig/contentConfig.ts
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css
🔇 Additional comments (11)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (5)

Line range hint 9-24: Remove redundant background-color property

The background-color is set twice in the .inputGroup styles. Remove the redundant declaration to avoid confusion.


25-30: LGTM! Clean input styling approach

The transparent background with flex properties allows for better control at the container level.


65-65: Fix incorrect height value for suffix/prefix

Setting height to 0 will hide the suffix/prefix content. This appears to be a bug.


140-147: LGTM! Proper error state handling

Good use of design system color tokens for error states.


157-159: LGTM! Accessible focus states

Good implementation of focus states with visible indicators.

app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/index.tsx (3)

1-17: LGTM! Clean import structure

The imports are well-organized and properly scoped for the new menu-based dial code selection functionality.


Line range hint 77-99: LGTM! Clean TextField implementation

The TextField component properly handles all necessary props and validation states.


29-75: 🛠️ Refactor suggestion

Refactor prefix rendering for better maintainability and accessibility

  1. The IIFE pattern makes the code harder to maintain. Consider extracting to a separate function.
  2. The selectedKeys logic uses dial_code instead of code, which is inconsistent.

Consider this refactor:

- const prefix = (function () {
+ const renderPrefix = () => {
   if (allowDialCodeChange) {
     return (
       <MenuTrigger>
         <Button
           color="neutral"
           icon="chevron-down"
           iconPosition="end"
           size="small"
           variant="subtle"
+          aria-label="Select country code"
         >
           {selectedCountry?.dial_code ?? ""}
         </Button>
         <Menu
           className={styles.currencyMenu}
           maxHeight={400}
           onAction={onMenuItemSelect}
           selectedKeys={
-            selectedCountry?.dial_code
+            selectedCountry?.code
               ? new Set([selectedCountry.code])
               : []
           }
           selectionMode="single"
+          aria-label="Country codes"
         >

Likely invalid or redundant comment.

app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx (3)

6-6: LGTM: Good choice using clsx

The addition of clsx for className composition follows React best practices.


26-26: LGTM: Clean className composition

The className composition using clsx is well-implemented, allowing for both default styles and custom classes.


22-25: Verify Popover component accepts maxHeight prop

The maxHeight prop is passed to Popover, but we should verify the Popover component's interface includes this prop.

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

♻️ Duplicate comments (1)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (1)

64-64: ⚠️ Potential issue

Fix incorrect height value for suffix/prefix

Setting height to 0 will hide the suffix/prefix content.

- height: 0;
+ height: 100%;
🧹 Nitpick comments (3)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (3)

21-22: Track TODO comment about overflow property

The comment indicates that overflow: hidden should be removed when max height is added. This should be tracked to prevent it from being forgotten.

Would you like me to create a GitHub issue to track the removal of overflow: hidden when max height is implemented?


Line range hint 72-84: Add comment explaining complex selector

The :has() selector handles multiple states elegantly, but its complexity warrants documentation for future maintenance.

+/* Applies hover styles only when input is not focused, readonly, disabled, or within a focused group */
.inputGroup[data-hovered]:has(
    > .input:not(
        :is(

168-180: Consider using CSS custom properties for calculations

The block-size calculations could be simplified by moving the formula to a custom property.

+:root {
+  --input-block-size: calc(
+    var(--body-line-height) + var(--body-margin-start) + var(--body-margin-end)
+  );
+}
 .inputGroup:has(> .input[data-size="small"]) {
-  block-size: calc(
-    var(--body-line-height) + var(--body-margin-start) + var(--body-margin-end)
-  );
+  block-size: var(--input-block-size);
   padding-block: var(--inner-spacing-2);
 }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f4cf381 and 36030b3.

📒 Files selected for processing (1)
  • app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (7 hunks)
🔇 Additional comments (3)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (3)

24-29: Clean and efficient input styling

Good separation of concerns by moving presentational styles to the container and keeping the input element styling minimal.


139-146: Well-structured invalid state handling

Good use of design system color tokens and proper state management for error conditions.


156-161: Accessible focus state implementation

Good implementation of focus states with proper visual indicators and state management.

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: 1

♻️ Duplicate comments (1)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (1)

63-63: ⚠️ Potential issue

Critical: Fix incorrect height value

Setting height to 0 will hide the suffix/prefix content.

- height: 0;
+ height: 100%;
🧹 Nitpick comments (3)
app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx (2)

Line range hint 13-25: LGTM: Clean implementation of ref handling and styling customization

The ref handling implementation is solid. Consider memoizing the mergedRef to prevent unnecessary re-renders.

- const mergedRef = mergeRefs(ref, localRef);
+ const mergedRef = useMemo(() => mergeRefs(ref, localRef), [ref]);
🧰 Tools
🪛 Biome (1.9.4)

[error] 28-28: 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] 28-28: 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)


53-57: Remove redundant Boolean check

The Boolean wrapper is unnecessary as the condition will be coerced to boolean automatically.

- {Boolean(prefix) && (
+ {prefix && (
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (1)

Line range hint 9-21: Remove TODO comment from production code

The comment about deleting overflow when max height is added should be tracked in an issue instead.

Would you like me to create an issue to track this TODO?

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 36030b3 and e4a075e.

📒 Files selected for processing (5)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBox.tsx (3 hunks)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBoxTrigger.tsx (2 hunks)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx (4 hunks)
  • app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBoxTrigger.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx

[error] 65-65: 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] 66-66: 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)

🔇 Additional comments (6)
app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx (2)

60-64: LGTM: Clean className composition

The className composition using clsx is well-implemented and maintains proper styling hierarchy.


72-76: Same Boolean redundancy as prefix section

Similar to the prefix section, the Boolean wrapper is unnecessary here.

- {Boolean(suffix) && (
+ {suffix && (
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (2)

23-28: LGTM: Clean input base styles

The input styles properly reset browser defaults while maintaining essential functionality.


Line range hint 71-179: Verify browser compatibility for :has selector

The :has selector is used extensively for state management. While it provides elegant solutions, ensure proper fallbacks for browsers that don't support it.

✅ Verification successful

:has selector usage is safe - data attributes provide fallback mechanism

The implementation already uses data attributes (data-focused, data-readonly, etc.) alongside :has selectors, which effectively serves as a built-in fallback mechanism. Modern browsers (Chrome 105+, Safari 15.4+, Firefox 103+) support :has, and older browsers can still rely on the data attributes for styling.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for :has selector usage and browser support
echo "Checking :has selector usage in CSS files..."
rg -l ":has\(" --type css

echo "Browser support for :has selector:"
curl -s "https://caniuse.com/api/features?feature=css-has" | jq '.data."css-has".support'

Length of output: 1236


Script:

#!/bin/bash
# Let's check the package.json for browserslist configuration and any polyfills
echo "Checking package.json for browser support configuration..."
fd package.json --exec cat {} \; | jq -r '.browserslist, .dependencies["@csstools/postcss-has-pseudo"]'

# Check for PostCSS configuration that might handle :has fallbacks
echo -e "\nChecking PostCSS configuration..."
fd "postcss.config" --exec cat {} \;

# Check if there are any CSS fallbacks implemented for :has
echo -e "\nChecking for potential CSS fallbacks..."
rg "\/\* @fallback|\[data-" --type css app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css

Length of output: 942


Script:

#!/bin/bash
# Let's check for data attributes that might be used as fallbacks
echo "Checking for data attributes in styles..."
rg "\[data-" app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css

# Check browser targets in root package.json
echo -e "\nChecking browser targets in root package.json..."
cat package.json | jq '.browserslist'

# Check for PostCSS config with fixed filename
echo -e "\nChecking PostCSS config..."
cat app/client/postcss.config.js 2>/dev/null || cat postcss.config.js 2>/dev/null

Length of output: 1369

app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBox.tsx (2)

11-11: LGTM: Clean import additions

The new imports for styles and theme hook are well-organized.

Also applies to: 14-14


32-32: Verify theme context availability

Ensure the component is wrapped in a ThemeProvider to avoid potential undefined theme values.

✅ Verification successful

Let me search for ThemeProvider usage and imports to verify the theme context availability. The previous ast-grep command didn't work well for this case, so I'm using ripgrep to search more broadly in the relevant directories.


Theme context is properly provided through @appsmith/wds-theming

The useTheme hook is imported from @appsmith/wds-theming which is the designated theming package. The implementation shows consistent theme usage across multiple components in the design system. No issues found with theme context availability.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for ThemeProvider usage in parent components
ast-grep --pattern 'ThemeProvider>$$$</ThemeProvider>'

Length of output: 130778


Script:

#!/bin/bash
# Search for ThemeProvider usage in parent components and imports
rg -A 3 "ThemeProvider|useTheme" app/client/packages/design-system/widgets/src/

# Check if theme is provided in test files
rg -A 3 "ThemeProvider" app/client/packages/design-system/widgets/src/**/*.test.* app/client/packages/design-system/widgets/src/**/*.spec.*

Length of output: 5334

Copy link

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

@jsartisan
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

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

♻️ Duplicate comments (2)
app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/index.tsx (2)

21-23: ⚠️ Potential issue

Handle potential undefined selectedCurrency

The previous review comment about handling undefined selectedCurrency is still valid.

Add a fallback or default currency:

  const selectedCurrency = CurrencyTypeOptions.find(
    (option) => option.currency === props.currencyCode,
-  );
+  ) ?? CurrencyTypeOptions[0]; // Fallback to first currency

25-29: ⚠️ Potential issue

Add proper type safety to onMenuItemSelect

The function still doesn't properly handle the undefined case for currency.

Add type safety:

  const onMenuItemSelect = (key: Key) => {
    const currency = CurrencyTypeOptions.find((option) => option.code === key);
-    onCurrencyChange(currency?.currency);
+    if (currency?.currency) {
+      onCurrencyChange(currency.currency);
+    }
  };
🧹 Nitpick comments (2)
app/client/packages/design-system/widgets/src/components/Popover/src/useRootContainer.ts (1)

6-8: Consider memoizing the initial DOM query

The DOM query is performed during state initialization and might be unnecessary if the theme provider element is not immediately available.

-const [root, setRoot] = useState<Element | undefined>(
-  document.body.querySelector("[data-theme-provider]") as Element,
-);
+const [root, setRoot] = useState<Element | undefined>();
app/client/packages/design-system/widgets/src/components/Datepicker/src/Datepicker.tsx (1)

Line range hint 83-85: Consider adding maxHeight prop for consistency

While the portal container change is good, consider adding the maxHeight prop to align with other components (Select, ComboBox) that use POPOVER_LIST_BOX_MAX_HEIGHT.

 <Popover
   UNSTABLE_portalContainer={root}
   className={clsx(datePickerStyles.popover, popoverClassName)}
+  maxHeight={POPOVER_LIST_BOX_MAX_HEIGHT}
 >
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4a075e and 508d3f8.

📒 Files selected for processing (11)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBox.tsx (3 hunks)
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Datepicker/src/Datepicker.tsx (2 hunks)
  • app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Menu/src/types.ts (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Popover/src/constants.ts (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Popover/src/index.ts (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Popover/src/useRootContainer.ts (1 hunks)
  • app/client/packages/design-system/widgets/src/components/Select/src/Select.tsx (3 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/index.tsx (1 hunks)
  • app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/index.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • app/client/packages/design-system/widgets/src/components/Popover/src/index.ts
  • app/client/packages/design-system/widgets/src/components/Popover/src/constants.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx
  • app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/component/index.tsx
  • app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css
  • app/client/packages/design-system/widgets/src/components/Menu/src/types.ts
🔇 Additional comments (5)
app/client/packages/design-system/widgets/src/components/Select/src/Select.tsx (1)

29-29: LGTM: Clean implementation of portal container and height constraints

The changes properly implement the root container management and height constraints for the popover.

Also applies to: 55-58

app/client/packages/design-system/widgets/src/components/ComboBox/src/ComboBox.tsx (1)

55-59: Verify CSS implementation for popover positioning

The changes look good and align with the previous review suggestions to use CSS for positioning.

✅ Verification successful

Let me analyze the CSS implementation further to ensure the positioning is correctly handled and consistent with the Select component.


CSS implementation for popover positioning is correctly implemented

The CSS implementation uses a proper transform with translateX and width calculation based on trigger width and inner spacing, which is a standard and reliable approach for popover positioning. The implementation is clean and follows best practices.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the CSS implementation for popover positioning
fd -e css -e scss . | xargs rg "comboboxPopover" -A 5

Length of output: 552


Script:

#!/bin/bash
# Check for any other CSS classes that might affect the popover positioning
rg "transform.*translate" -A 2 -B 2 app/client/packages/design-system/widgets/src/components/ComboBox/src/styles.module.css

# Check if there are any other CSS files that might affect the ComboBox positioning
fd -e css -e scss . | xargs rg "transform.*translate" -A 2 -B 2

# Check the Select component's CSS for comparison
fd -e css -e scss . | xargs rg "selectPopover" -A 2 -B 2

Length of output: 70177

app/client/packages/design-system/widgets/src/components/Datepicker/src/Datepicker.tsx (2)

8-8: LGTM: Clean import addition

The useRootContainer import is properly grouped with other @appsmith/wds imports.


48-48: Verify consistent hook usage across components

The useRootContainer implementation aligns with the design system's standardization efforts.

✅ Verification successful

useRootContainer hook is consistently used across components

The hook is correctly implemented in all relevant components: Menu, Datepicker, ComboBox, and Select, following the same pattern of usage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify consistent usage of useRootContainer across components
ast-grep --pattern 'const $_ = useRootContainer()' 'app/client/packages/design-system'

Length of output: 562

app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/index.tsx (1)

20-20: Verify form integration

Let's ensure this component properly integrates with form validation and submission.

✅ Verification successful

Let me search for the component usage and form integration patterns to verify this properly.


The previous results show the component is used in forms, particularly in JSONFormWidget. Let me verify the specific implementation details to ensure proper integration.


Form integration is properly implemented

The component is correctly integrated with the form system through JSONFormWidget, with proper currency change handling, value transformation, and validation support. The changes to allowCurrencyChange and onCurrencyChange props align with the existing form integration patterns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for form wrapper usage of this component
ast-grep --pattern 'CurrencyInputComponent $props={$$$}'

# Search for form validation handling
rg -l "validationStatus.*CurrencyInputComponent" -g "*.{ts,tsx}"

Length of output: 125


Script:

#!/bin/bash
# Search for the component usage with a more flexible pattern
rg -l "CurrencyInputWidget" -g "*.{ts,tsx}"

# Search for the component's form integration
ast-grep --pattern 'const $_ = useForm($$$)'

# Look for currency-related form validation
rg "currency.*validation|validation.*currency" -g "*.{ts,tsx}" -i

Length of output: 5338


Script:

#!/bin/bash
# Check the specific component implementation for form integration
rg -A 10 "export.*CurrencyInputComponent" app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/component/index.tsx

# Look for JSONFormWidget integration
rg -A 10 "CurrencyInputField" app/client/src/widgets/JSONFormWidget/fields/CurrencyInputField.tsx

Length of output: 1842

Copy link

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

@jsartisan
Copy link
Contributor Author

@KelvinOm added on useRootContainer hook as well and used it in select, menu, combobox.

@jsartisan jsartisan requested a review from KelvinOm December 18, 2024 08:35
Copy link

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

Copy link
Contributor

@ichik ichik left a comment

Choose a reason for hiding this comment

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

Needs a follow-up to address the fact that Windows doesn't provide flag emojis.

@jsartisan jsartisan added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Dec 18, 2024
@jsartisan jsartisan merged commit 65b98c6 into release Dec 19, 2024
66 of 72 checks passed
@jsartisan jsartisan deleted the chore/fix-wds-bugs branch December 19, 2024 05:42
NandanAnantharamu pushed a commit that referenced this pull request Dec 27, 2024
![CleanShot 2024-12-16 at 16 42
37](https://github.com/user-attachments/assets/1b59a2f9-d8d1-4fa9-8827-493622088e56)

![CleanShot 2024-12-16 at 16 43
12](https://github.com/user-attachments/assets/b8d29bba-55c5-42ec-8352-fb8b374ef8d2)

Fixes #38091 

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

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

## Summary by CodeRabbit

- **New Features**
- Added new props for enhanced styling and functionality in various
components, including `className` and `maxHeight`.
- Introduced dropdown menus for currency and ISD code selection in the
CurrencyInput and PhoneInput components.
- New story added for the `TextField` component showcasing prefix and
suffix usage.
- Added a custom hook `useRootContainer` for better theme provider
management.

- **Bug Fixes**
- Improved conditional rendering for input elements based on prop
values.

- **Style**
- Significant updates to CSS for input and select components, enhancing
layout and visual feedback.
- New CSS classes added for ComboBox and styling adjustments for
existing classes.
	- Enhanced styling for the ComboBox popover and other components.
- New CSS rules for currency and dial code options in respective
widgets.

- **Documentation**
- Updated property configurations for widgets to include new toggle
options for currency and dial code changes.
<!-- 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/12391730072>
> Commit: b8bb738
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12391730072&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Anvil`
> Spec:
> <hr>Wed, 18 Dec 2024 11:33:31 UTC
<!-- end of auto-generated comment: Cypress test results  -->
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Feb 7, 2025
![CleanShot 2024-12-16 at 16 42
37](https://github.com/user-attachments/assets/1b59a2f9-d8d1-4fa9-8827-493622088e56)

![CleanShot 2024-12-16 at 16 43
12](https://github.com/user-attachments/assets/b8d29bba-55c5-42ec-8352-fb8b374ef8d2)

Fixes appsmithorg#38091 

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

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

## Summary by CodeRabbit

- **New Features**
- Added new props for enhanced styling and functionality in various
components, including `className` and `maxHeight`.
- Introduced dropdown menus for currency and ISD code selection in the
CurrencyInput and PhoneInput components.
- New story added for the `TextField` component showcasing prefix and
suffix usage.
- Added a custom hook `useRootContainer` for better theme provider
management.

- **Bug Fixes**
- Improved conditional rendering for input elements based on prop
values.

- **Style**
- Significant updates to CSS for input and select components, enhancing
layout and visual feedback.
- New CSS classes added for ComboBox and styling adjustments for
existing classes.
	- Enhanced styling for the ComboBox popover and other components.
- New CSS rules for currency and dial code options in respective
widgets.

- **Documentation**
- Updated property configurations for widgets to include new toggle
options for currency and dial code changes.
<!-- 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/12391730072>
> Commit: b8bb738
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12391730072&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Anvil`
> Spec:
> <hr>Wed, 18 Dec 2024 11:33:31 UTC
<!-- end of auto-generated comment: Cypress test results  -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Anvil Pod Issue related to Anvil project ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog Task A simple Todo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Currency and Country Code change
4 participants