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

feat: editable ide tabs #36665

Open
wants to merge 25 commits into
base: release
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
4c4694f
perf: do not mutate history for the same url
alex-golovanov Sep 27, 2024
295a041
chore: add usehooks-ts
alex-golovanov Sep 27, 2024
144edb7
perf: correct state styles & fix spacing
alex-golovanov Oct 2, 2024
8f09c44
perf: add input ref & remove redundant callback prop
alex-golovanov Oct 2, 2024
6924378
chore: fix typo
alex-golovanov Oct 2, 2024
6f6bc3d
feat: hook for common name editing needs
alex-golovanov Oct 2, 2024
195daa7
feat: adde editable tab name
alex-golovanov Oct 2, 2024
70add12
chore: remove unused files
alex-golovanov Oct 2, 2024
2d6bc28
Merge branch 'release' into feat/32440-editable-ide-tabs
alex-golovanov Oct 2, 2024
b98f363
fix: replace lock
alex-golovanov Oct 2, 2024
f2c6924
perf: tweak positioning
alex-golovanov Oct 2, 2024
aa4c87e
fix: stop event propagation on tab close
alex-golovanov Oct 2, 2024
d6cc17a
fix: validation reset on escape
alex-golovanov Oct 2, 2024
df31a7a
perf: replace tw with styled & tweak styles
alex-golovanov Oct 3, 2024
072f1d6
perf: lifted state into editable component for perf
alex-golovanov Oct 4, 2024
1d128f6
chore: add usehooks-ts to jest config
alex-golovanov Oct 7, 2024
95f041e
chore: add forwardRef & rename striked to strikethrough for consistency
alex-golovanov Oct 7, 2024
ede76cd
chore: move test constants to a file
alex-golovanov Oct 7, 2024
bfadb0b
test: add tab tests
alex-golovanov Oct 7, 2024
47ee66b
chore: correct typing and story
alex-golovanov Oct 7, 2024
d05671f
fix: revert tab close button name change
alex-golovanov Oct 7, 2024
a47adb5
chore: fix a linter error
alex-golovanov Oct 7, 2024
b1f6a7c
fix: addressed focusOut related issues
alex-golovanov Oct 10, 2024
14a6c75
Merge branch 'release' into feat/32440-editable-ide-tabs
alex-golovanov Oct 10, 2024
dc89acb
Merge branch 'release' into feat/32440-editable-ide-tabs
alex-golovanov Oct 10, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@
"typescript": "^5.5.4",
"unescape-js": "^1.1.4",
"url-search-params-polyfill": "^8.0.0",
"usehooks-ts": "^3.1.0",
"uuid": "^9.0.0",
"validate-color": "^2.2.4",
"web-vitals": "3.5.2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,11 +191,8 @@ export const StyledEditableInput = styled.input`
border: 1px solid transparent;
border-radius: var(--ads-v2-border-radius);
outline: none;
padding: 0;
margin: 0;
position: absolute;
left: -3px;
top: -3px;
width: 100%;
padding: var(--ads-v2-spaces-1);

Expand All @@ -205,6 +202,6 @@ export const StyledEditableInput = styled.input`

&:focus,
&:active {
border-color: var(--ads-v2-colors-control-field-default-border);
border-color: var(--ads-v2-colors-control-field-active-border);
}
`;
8 changes: 2 additions & 6 deletions app/client/packages/design-system/ads/src/Text/Text.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ function Text({
className,
color,
inputProps,
inputRef,
isEditable,
kind,
onChange,
renderAs,
...rest
}: TextProps) {
Expand All @@ -35,11 +35,7 @@ function Text({
{...rest}
>
{isEditable && typeof children === "string" ? (
<StyledEditableInput
onChange={onChange}
value={children}
{...inputProps}
/>
<StyledEditableInput ref={inputRef} value={children} {...inputProps} />
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved
) : (
children
)}
Expand Down
4 changes: 2 additions & 2 deletions app/client/packages/design-system/ads/src/Text/Text.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,13 @@ export type TextProps = {
isStriked?: boolean;
/** whether the text is editable or not */
isEditable?: boolean;
/** onChange event for editable text */
onChange?: (event: React.ChangeEvent<HTMLInputElement>) => void;
/** input component props while isEditable is true */
inputProps?: Omit<
React.InputHTMLAttributes<HTMLInputElement>,
"value" | "onChange"
>;
/** ref for input component */
inputRef?: React.RefObject<HTMLInputElement>;
} & React.HTMLAttributes<HTMLLabelElement> &
React.HTMLAttributes<HTMLHeadingElement> &
React.HTMLAttributes<HTMLParagraphElement> &
Expand Down
99 changes: 0 additions & 99 deletions app/client/src/IDE/Components/FileTab.tsx

This file was deleted.

162 changes: 162 additions & 0 deletions app/client/src/IDE/Components/FileTab/FileTab.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
import React, { useEffect, useMemo, useRef, useState } from "react";

import clsx from "classnames";
import { noop } from "lodash";

import { Icon, Spinner, Tooltip } from "@appsmith/ads";
import { sanitizeString } from "utils/URLUtils";
import { useBoolean, useEventCallback, useOnClickOutside } from "usehooks-ts";
import { usePrevious } from "@mantine/hooks";
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved

import * as Styled from "./styles";

interface FileTabProps {
isActive: boolean;
isLoading?: boolean;
title: string;
onClick: () => void;
onClose: (e: React.MouseEvent) => void;
icon?: React.ReactNode;
editorConfig?: {
/** Triggered on enter or click outside */
onTitleSave: (name: string) => void;
/** Used to normalize title (remove white spaces etc.) */
titleTransformer: (name: string) => string;
/** Validates title and returns an error message or null */
validateTitle: (name: string) => string | null;
};
}

export const FileTab = ({
editorConfig,
icon,
isActive,
isLoading = false,
onClick,
onClose,
title,
}: FileTabProps) => {
const {
setFalse: exitEditMode,
setTrue: enterEditMode,
value: isEditing,
} = useBoolean(false);

const previousTitle = usePrevious(title);
const [editableTitle, setEditableTitle] = useState(title);
const currentTitle =
isEditing || isLoading || title !== editableTitle ? editableTitle : title;
const [validationError, setValidationError] = useState<string | null>(null);
const inputRef = useRef<HTMLInputElement>(null);

const attemptToSave = () => {
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved
if (editorConfig) {
const { onTitleSave, validateTitle } = editorConfig;
const nameError = validateTitle(editableTitle);

if (nameError !== null) {
setValidationError(nameError);
} else {
exitEditMode();
onTitleSave(editableTitle);
}
}
};
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved

const handleKeyUp = useEventCallback(
(e: React.KeyboardEvent<HTMLInputElement>) => {
if (e.key === "Enter") {
attemptToSave();
} else if (e.key === "Escape") {
exitEditMode();
setEditableTitle(title);
} else {
setValidationError(null);
}
},
);

useOnClickOutside(inputRef, () => {
attemptToSave();
});

const handleTitleChange = useEventCallback(
(e: React.ChangeEvent<HTMLInputElement>) => {
setEditableTitle(
editorConfig
? editorConfig.titleTransformer(e.target.value)
: e.target.value,
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved
);
},
);

const handleEnterEditMode = useEventCallback(() => {
setEditableTitle(title);
enterEditMode();
});

const handleDoubleClick = editorConfig ? handleEnterEditMode : noop;

const inputProps = useMemo(
() => ({
onKeyUp: handleKeyUp,
onChange: handleTitleChange,
autofocus: true,
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved
style: {
padding: "0 var(--ads-v2-spaces-1)",
},
}),
[handleKeyUp, handleTitleChange],
);

useEffect(() => {
if (!isEditing && previousTitle !== title) {
setEditableTitle(title);
}
}, [title, previousTitle, isEditing]);

// this is a nasty hack to re-focus the input after context retention applies the focus
// it will be addressed soon, likely by a focus retention modification
useEffect(() => {
const input = inputRef.current;

if (isEditing && input) {
setTimeout(() => {
input.focus();
}, 200);
}
}, [isEditing]);
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved

return (
<Styled.Tab
className={clsx("editor-tab", isActive && "active")}
data-testid={`t--ide-tab-${sanitizeString(title)}`}
onClick={onClick}
onDoubleClick={handleDoubleClick}
>
{icon && !isLoading ? (
<Styled.IconContainer>{icon}</Styled.IconContainer>
) : null}
{isLoading && <Spinner size="sm" />}

<Tooltip content={validationError} visible={Boolean(validationError)}>
<Styled.Text
inputProps={inputProps}
inputRef={inputRef}
isEditable={isEditing}
kind="body-s"
>
{currentTitle}
</Styled.Text>
</Tooltip>

{/* not using button component because of the size not matching design */}
<Icon
className="tab-close rounded-[4px] hover:bg-[var(--ads-v2-colors-action-tertiary-surface-hover-bg)] cursor-pointer p-[2px]"
data-testid="t--tab-close-btn"
name="close-line"
onClick={onClose}
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved
/>
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

Enhance maintainability by moving inline styles to styled components.

Inline styles within the className prop can make the code harder to maintain. Let's consider moving these styles to a styled component for better readability and consistency.

You can create a styled component for the close icon:

// In styles.ts
export const CloseIcon = styled(Icon)`
  border-radius: 4px;
  padding: 2px;
  cursor: pointer;

  &:hover {
    background-color: var(--ads-v2-colors-action-tertiary-surface-hover-bg);
  }
`;

Then update your JSX:

- <Icon
-   className="tab-close rounded-[4px] hover:bg-[var(--ads-v2-colors-action-tertiary-surface-hover-bg)] cursor-pointer p-[2px]"
+ <Styled.CloseIcon
    data-testid="t--tab-close-btn"
    name="close-line"
    onClick={onClose}
  />

</Styled.Tab>
);
};
1 change: 1 addition & 0 deletions app/client/src/IDE/Components/FileTab/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { FileTab } from "./FileTab";
61 changes: 61 additions & 0 deletions app/client/src/IDE/Components/FileTab/styles.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import styled from "styled-components";

import { Text as ADSText } from "@appsmith/ads";

export const Tab = styled.div`
display: flex;
height: 100%;
position: relative;
font-size: 12px;
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

Remember to use design tokens for font sizes.

Class, on line 9, we have the font-size set directly to 12px. It's important to utilize our design system variables to maintain consistency throughout the application.

Let's adjust it as follows:

-  font-size: 12px;
+  font-size: var(--ads-v2-font-size-3);

Ensure that --ads-v2-font-size-3 corresponds to the appropriate size defined in our design system.

📝 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
font-size: 12px;
font-size: var(--ads-v2-font-size-3);

color: var(--ads-v2-colors-text-default);
cursor: pointer;
gap: var(--ads-v2-spaces-2);
border-top: 1px solid transparent;
border-top-left-radius: var(--ads-v2-border-radius);
border-top-right-radius: var(--ads-v2-border-radius);
align-items: center;
justify-content: center;
padding: var(--ads-v2-spaces-3);
padding-top: 6px; // to accommodate border and make icons align correctly
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved
border-left: 1px solid transparent;
border-right: 1px solid transparent;
border-top: 2px solid transparent;

&.active {
background: var(--ads-v2-colors-control-field-default-bg);
border-top-color: var(--ads-v2-color-bg-brand);
border-left-color: var(--ads-v2-color-border-muted);
border-right-color: var(--ads-v2-color-border-muted);
}

& > .tab-close {
position: relative;
right: -2px;
visibility: hidden;
}

&:hover > .tab-close {
visibility: visible;
}

&.active > .tab-close {
visibility: visible;
}
`;
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved

export const IconContainer = styled.div`
height: 12px;
width: 12px;
alex-golovanov marked this conversation as resolved.
Show resolved Hide resolved
display: flex;
align-items: center;
justify-content: center;
flex-shrink: 0;
img {
width: 12px;
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

Ensure consistent icon sizing using container dimensions.

Class, on line 51, the img width is set to 12px. Since the IconContainer already defines the size, we can make the image fit the container.

Adjust it as follows:

-      width: 12px;
+      width: 100%;
+      height: 100%;

This ensures the icon scales appropriately within its container.

Committable suggestion was skipped due to low confidence.

}
`;

export const Text = styled(ADSText)`
min-width: 3ch;
padding: 0 var(--ads-v2-spaces-1);
`;
Loading
Loading