Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
@@ -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>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* Reason for using div.selectInputGroup instead of just .selectInputGroup is to increase the specificity of the selector
* Also, we are removing the padding-inline from selectInputGroup and adding it to selectTriggerButton
* This is done to ensure that width of the select popover is same as the width of the select trigger button. Otherwise,
* the width of the popover will be less than the width of the trigger button because of the padding-inline by .inputGroup .
*/
div.selectInputGroup {
padding-inline: 0;
}

button.selectTriggerButton {
display: flex;
align-items: center;
padding-inline: var(--inner-spacing-2);
}

.selectTriggerButton:has(> [data-select-text]) {
block-size: var(--body-line-height);
}

.selectTriggerButton [data-select-text] {
display: flex;
align-items: center;
flex: 1;
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export const WithPrefixAndSuffix: Story = {
prefix={<Icon name="user" size="medium" />}
suffix={<Icon name="user" size="medium" />}
/>
<TextField prefix={<>+121212</>} />
</Flex>
),
};
Expand Down