Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import clsx from "clsx";
import React, { useMemo } from "react";
import { getTypographyClassName } from "@appsmith/wds-theming";
import { Spinner, textInputStyles, Input, IconButton } from "@appsmith/wds";
import { Spinner, Input, IconButton } from "@appsmith/wds";

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

interface ComboBoxTriggerProps {
Expand All @@ -20,6 +19,7 @@ export const ComboBoxTrigger: React.FC<ComboBoxTriggerProps> = (props) => {

return (
<IconButton
className={styles.comboboxTriggerButton}
icon="chevron-down"
isDisabled={isDisabled}
size={size === "medium" ? "small" : "xSmall"}
Expand All @@ -29,7 +29,8 @@ export const ComboBoxTrigger: React.FC<ComboBoxTriggerProps> = (props) => {

return (
<Input
className={clsx(textInputStyles.input, getTypographyClassName("body"))}
className={styles.comboboxInput}
inputGroupClassName={styles.comboboxInputGroup}
placeholder={placeholder}
size={size}
suffix={suffix}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
div.comboboxInputGroup {

Copy link
Copy Markdown
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.

padding-inline: 0;
}

input.comboboxInput {
padding-inline: var(--inner-spacing-2);
}

.comboboxTriggerButton {
margin-inline-end: var(--inner-spacing-2);
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import type { InputProps } from "./types";

function _Input(props: InputProps, ref: React.Ref<HTMLInputElement>) {
const {
className,
defaultValue,
inputGroupClassName,
isLoading,
isReadOnly,
prefix,
Expand Down Expand Up @@ -45,18 +47,22 @@ function _Input(props: InputProps, ref: React.Ref<HTMLInputElement>) {
})();

return (
<Group className={styles.inputGroup}>
<Group className={clsx(styles.inputGroup, inputGroupClassName)}>
{Boolean(prefix) && <span data-input-prefix>{prefix}</span>}
<HeadlessInput
{...rest}
className={clsx(styles.input, getTypographyClassName("body"))}
className={clsx(
styles.input,
getTypographyClassName("body"),
className,
)}
data-readonly={Boolean(isReadOnly) ? true : undefined}
data-size={Boolean(size) ? size : undefined}
defaultValue={defaultValue}
ref={ref}
type={showPassword ? "text" : type}
value={isEmpty && Boolean(isReadOnly) ? "—" : value}
/>
{Boolean(prefix) && <span data-input-prefix>{prefix}</span>}
{Boolean(suffix) && <span data-input-suffix>{suffix}</span>}
</Group>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,8 @@
width: 100%;
}

.input {
position: relative;
display: flex;
flex: 1;
align-items: center;
box-sizing: content-box;
.inputGroup {
background-color: var(--color-bg);

Copy link
Copy Markdown
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

max-inline-size: 100%;
padding-block: var(--inner-spacing-1);
padding-inline: var(--inner-spacing-2);
Expand All @@ -25,8 +21,12 @@
overflow: hidden;
}

.input:has(> [data-select-text]) {
block-size: var(--body-line-height);
.input {
border: none;
outline: none;
background-color: transparent;
flex: 1;
padding: 0;
}

.input:is(textarea) {
Expand Down Expand Up @@ -57,59 +57,27 @@
);
}

.inputGroup:has(> [data-input-prefix]) .input {
padding-left: var(--sizing-8);
}

.inputGroup:has(> [data-input-prefix]) .input[data-size="large"] {
padding-left: var(--sizing-12);
}

.inputGroup:has(> [data-input-prefix]) .input[data-size="small"] {
padding-left: var(--sizing-6);
}

.inputGroup:has(> [data-input-prefix]) [data-input-prefix] {
left: var(--inner-spacing-1);
position: absolute;
}

.inputGroup:has(> [data-input-suffix]) .input {
padding-right: var(--sizing-8);
}

.inputGroup:has(> [data-input-suffix]) .input[data-size="large"] {
padding-right: var(--sizing-12);
}

.inputGroup:has(> [data-input-suffix]) .input[data-size="small"] {
padding-right: var(--sizing-6);
}

.inputGroup:has(> [data-input-suffix]) [data-input-suffix] {
right: var(--inner-spacing-1);
position: absolute;
}

.inputGroup :is([data-input-suffix], [data-input-prefix]) {
display: flex;
justify-content: center;
align-items: center;
height: 0;
Comment thread
jsartisan marked this conversation as resolved.
}

/**
* ----------------------------------------------------------------------------
* HOVERED
* ----------------------------------------------------------------------------
*/
.inputGroup[data-hovered]
.input:not(
:is(
[data-focused],
[data-readonly],
[data-disabled],
[data-focus-within],
:has(~ input[data-disabled="true"])
.inputGroup[data-hovered]:has(
Comment thread
KelvinOm marked this conversation as resolved.
> .input:not(
:is(
[data-focused],
[data-readonly],
[data-disabled],
[data-focus-within],
:has(~ input[data-disabled="true"])
)
)
) {
background-color: var(--color-bg-neutral-subtle-hover);
Expand Down Expand Up @@ -157,8 +125,8 @@
* DISABLED
* ----------------------------------------------------------------------------
*/
.input[data-disabled],
.input:has(~ input[data-disabled]) {
.inputGroup:has(> .input[data-disabled]),
.inputGroup:has(> .input:has(~ input[data-disabled])) {
cursor: not-allowed;
box-shadow: none;
}
Expand All @@ -168,13 +136,14 @@
* INVALID
* ----------------------------------------------------------------------------
*/
.input[data-invalid] {
.inputGroup:has(> .input[data-invalid]) {
box-shadow: 0 0 0 1px var(--color-bd-negative);
}

.inputGroup[data-hovered]
.input[data-invalid]:not(
:is([data-focused], [data-readonly], [data-disabled])
.inputGroup[data-hovered]:has(
> .input[data-invalid]:not(
:is([data-focused], [data-readonly], [data-disabled])
)
) {
box-shadow: 0 0 0 1px var(--color-bd-negative-hover);
}
Expand All @@ -184,7 +153,9 @@
* FOCUSSED
* ----------------------------------------------------------------------------
*/
.input:is([data-focused], [data-focus-within]):not([data-readonly]) {
.inputGroup:has(
> .input:is([data-focused], [data-focus-within]):not([data-readonly])
) {
background-color: transparent;
box-shadow: 0 0 0 2px var(--color-bd-focus);
}
Expand All @@ -194,31 +165,17 @@
* SIZE
* ----------------------------------------------------------------------------
*/
.input[data-size="small"] {
.inputGroup:has(> .input[data-size="small"]) {
block-size: calc(
var(--body-line-height) + var(--body-margin-start) + var(--body-margin-end)
);
padding-block: var(--inner-spacing-2);
}

.input[data-size="large"] {
.inputGroup:has(> .input[data-size="large"]) {
block-size: calc(
var(--body-line-height) + var(--body-margin-start) + var(--body-margin-end)
);
padding-block: var(--inner-spacing-3);
padding-inline: var(--inner-spacing-3);
}

/**
* ----------------------------------------------------------------------------
Comment thread
KelvinOm marked this conversation as resolved.
* SELECT BUTTON's TEXT
* ----------------------------------------------------------------------------
*/
.input [data-select-text] {
display: flex;
align-items: center;
}

.input [data-select-text] [data-icon] {
margin-inline-end: var(--inner-spacing-1);
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ interface CommonInputProps {

export interface InputProps
extends Omit<HeadlessInputProps, "prefix" | "size">,
CommonInputProps {}
CommonInputProps {
inputGroupClassName?: string;
}

export interface TextAreaInputProps
extends Omit<HeadlessTextAreaProps, "prefix" | "size">,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ import React, { createContext, useContext } from "react";
import { listStyles, Popover } from "@appsmith/wds";
import { Menu as HeadlessMenu } from "react-aria-components";

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

const MenuNestingContext = createContext(0);

export const Menu = (props: MenuProps) => {
const { children } = props;
const { children, maxHeight } = props;
const root = document.body.querySelector(
"[data-theme-provider]",
) as HTMLButtonElement;
Expand All @@ -18,7 +19,11 @@ export const Menu = (props: MenuProps) => {
return (
<MenuNestingContext.Provider value={nestingLevel + 1}>
{/* Only the parent Popover should be placed in the root. Placing child popoves in root would cause the menu to function incorrectly */}
<Popover UNSTABLE_portalContainer={isRootMenu ? root : undefined}>
<Popover
UNSTABLE_portalContainer={isRootMenu ? root : undefined}
className={styles.menuPopover}
maxHeight={maxHeight}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this implemented only for menu? Don't we need this for other lists? It may also be worth considering a default constant for this. What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added POPOVER_LIST_BOX_MAX_HEIGHT and used it in menu, select and combobox.

>
<HeadlessMenu className={listStyles.listBox} {...props}>
{children}
</HeadlessMenu>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,7 @@
);
}
}

.menuPopover {
overflow-y: auto;
}

Copy link
Copy Markdown
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.

Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import type { MenuProps as AriaMenuProps } from "react-aria-components";

export interface MenuProps
extends Omit<
AriaMenuProps<object>,
"slot" | "selectionMode" | "selectedKeys"
> {}
export interface MenuProps extends Omit<AriaMenuProps<object>, "slot"> {
maxHeight?: number;
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ export const Main: Story = {
<MenuTrigger>
<Button>Open The Menu…</Button>
<Menu>
<MenuItem id="1">Item 1</MenuItem>
<MenuItem id="2">Item 2</MenuItem>
<MenuItem id="3">Item 3</MenuItem>
<MenuItem id="4">Item 4</MenuItem>
{Array.from({ length: 100 }, (_, i) => (
<MenuItem id={String(i + 1)} key={i}>
Item {i + 1}
</MenuItem>
))}
Comment thread
jsartisan marked this conversation as resolved.
Outdated
<SubmenuTrigger>
<MenuItem id="5">Submenu</MenuItem>
<Menu>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,16 @@ import {
import { Icon, Text, listBoxItemStyles } from "@appsmith/wds";

import type { MenuItemProps } from "./types";
import clsx from "clsx";

export function MenuItem(props: MenuItemProps) {
const { children, icon, ...rest } = props;
const { children, className, icon, ...rest } = props;

return (
<HeadlessMenuItem {...rest} className={listBoxItemStyles.listBoxItem}>
<HeadlessMenuItem
{...rest}
className={clsx(listBoxItemStyles.listBoxItem, className)}
>
{composeRenderProps(children, (children, { hasSubmenu }) => (
<>
{icon && <Icon name={icon} />}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import clsx from "clsx";
import React from "react";
import { Icon, Spinner, textInputStyles } from "@appsmith/wds";
import { getTypographyClassName } from "@appsmith/wds-theming";
import { Button, Group, SelectValue } from "react-aria-components";

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

interface SelectTriggerProps {
Expand All @@ -17,9 +19,11 @@ export const SelectTrigger: React.FC<SelectTriggerProps> = (props) => {
const { isDisabled, isInvalid, isLoading, placeholder, size } = props;

return (
<Group className={textInputStyles.inputGroup}>
<Group
className={clsx(textInputStyles.inputGroup, styles.selectInputGroup)}
>
<Button
className={textInputStyles.input}
className={clsx(textInputStyles.input, styles.selectTriggerButton)}
data-invalid={Boolean(isInvalid) ? "" : undefined}
data-size={size}
isDisabled={isDisabled}
Expand All @@ -32,10 +36,14 @@ export const SelectTrigger: React.FC<SelectTriggerProps> = (props) => {
isPlaceholder ? placeholder : defaultChildren
}
</SelectValue>
<span data-input-suffix>
{Boolean(isLoading) ? (
<Spinner />
) : (
<Icon name="chevron-down" size="medium" />
)}
</span>
Comment on lines +37 to +45

Copy link
Copy Markdown
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)

</Button>
<span data-input-suffix>
{Boolean(isLoading) ? <Spinner /> : <Icon name="chevron-down" />}
</span>
</Group>
);
};
Loading