Skip to content

fix: empty space is clickable when user edits app name or organisatio… #36882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Sai6347 wants to merge 1 commit into from
Closed

fix: empty space is clickable when user edits app name or organisatio… #36882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 49 additions & 5 deletions app/client/packages/design-system/ads-old/src/EditableTextSubComponent/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const TextContainer = styled.div<{
}>`
display: flex;
align-items: center;
// justify-content: space-evenly;
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider removing commented-out code

Dear student, it's generally a good practice to remove any commented-out code like // justify-content: space-evenly; unless it's needed for future reference. This helps keep the codebase clean and maintainable.

.bp3-editable-text.bp3-editable-text-editing::before,
.bp3-editable-text.bp3-disabled::before {
display: none;
Expand All @@ -80,7 +81,6 @@ const TextContainer = styled.div<{
color: var(--ads-editable-text-subcomponent-default-text-color);
overflow: hidden;
text-overflow: ellipsis;
${(props) => (props.isEditing ? "display: none" : "display: block")};
width: fit-content !important;
min-width: auto !important;
line-height: inherit !important;
Expand Down Expand Up @@ -138,6 +138,7 @@ export const EditableTextSubComponent = React.forwardRef(
const [value, setValue] = useState(defaultValue);
const [lastValidValue, setLastValidValue] = useState(defaultValue);
const [changeStarted, setChangeStarted] = useState<boolean>(false);
const [isCancelled, setIsCancelled] = useState<boolean>(false);

useEffect(() => {
if (isError) {
Expand Down Expand Up @@ -187,6 +188,8 @@ export const EditableTextSubComponent = React.forwardRef(

if (finalVal && finalVal !== defaultValue) {
onBlur && onBlur(finalVal);
setLastValidValue(finalVal);
setValue(finalVal);
}

setIsEditing(false);
Expand Down Expand Up @@ -215,17 +218,31 @@ export const EditableTextSubComponent = React.forwardRef(
const error = errorMessage ? errorMessage : false;

if (!error && finalVal !== "") {
setLastValidValue(finalVal);
onTextChanged && onTextChanged(finalVal);
}

setValue(finalVal);
setIsInvalid(error);
setChangeStarted(true);
},
[inputValidation, onTextChanged],
[inputValidation, onTextChanged, valueTransform],
);

const onCancel = useCallback(() => {
onBlur && onBlur(lastValidValue);
setIsEditing(false);
setIsInvalid(false);
setSavingState(SavingState.NOT_STARTED);
setValue(lastValidValue);
setIsCancelled(true);
}, [lastValidValue, onBlur]);
Comment on lines +231 to +238
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify function calls with optional chaining

In your onCancel function, you can simplify the conditional invocation of onBlur by using optional chaining. This makes the code cleaner and more readable.

Here's how you can apply this change:

-          onBlur && onBlur(lastValidValue);
+          onBlur?.(lastValidValue);
📝 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
const onCancel = useCallback(() => {
onBlur && onBlur(lastValidValue);
setIsEditing(false);
setIsInvalid(false);
setSavingState(SavingState.NOT_STARTED);
setValue(lastValidValue);
setIsCancelled(true);
}, [lastValidValue, onBlur]);
const onCancel = useCallback(() => {
onBlur?.(lastValidValue);
setIsEditing(false);
setIsInvalid(false);
setSavingState(SavingState.NOT_STARTED);
setValue(lastValidValue);
setIsCancelled(true);
}, [lastValidValue, onBlur]);
🧰 Tools
🪛 Biome

[error] 232-232: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


useEffect(() => {
if (isCancelled) {
setValue(lastValidValue);
}
}, [isCancelled, lastValidValue]);

Comment on lines +240 to +245
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Review the necessity of useEffect for isCancelled

I notice that you're setting setValue(lastValidValue); both in the onCancel function and within a useEffect that watches isCancelled. This might be redundant. Let's consider simplifying the state management by removing the isCancelled state and the accompanying useEffect.

You can modify your code as follows:

- const [isCancelled, setIsCancelled] = useState<boolean>(false);

  // ...

  const onCancel = useCallback(() => {
-   onBlur && onBlur(lastValidValue);
+   onBlur?.(lastValidValue);
    setIsEditing(false);
    setIsInvalid(false);
    setSavingState(SavingState.NOT_STARTED);
    setValue(lastValidValue);
-   setIsCancelled(true);
  }, [lastValidValue, onBlur]);

- useEffect(() => {
-   if (isCancelled) {
-     setValue(lastValidValue);
-   }
- }, [isCancelled, lastValidValue]);
📝 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
useEffect(() => {
if (isCancelled) {
setValue(lastValidValue);
}
}, [isCancelled, lastValidValue]);
const onCancel = useCallback(() => {
onBlur?.(lastValidValue);
setIsEditing(false);
setIsInvalid(false);
setSavingState(SavingState.NOT_STARTED);
setValue(lastValidValue);
}, [lastValidValue, onBlur]);

const iconName =
!isEditing &&
savingState === SavingState.NOT_STARTED &&
Expand All @@ -251,15 +268,42 @@ export const EditableTextSubComponent = React.forwardRef(
className={props.className}
disabled={!isEditing}
isEditing={isEditing}
onCancel={onConfirm}
onChange={onInputchange}
onConfirm={onConfirm}
placeholder={props.placeholder || defaultValue}
ref={ref}
selectAllOnFocus
value={value}
/>

{value && !props.hideEditIcon && isEditing ? (
<>
<span
className="pl-1"
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
onConfirm(value);
}}
>
<Icon className="cursor-pointer" name="check-line" size="md" />
</span>
<span
className="px-1"
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
onCancel();
}}
>
<Icon
className="cursor-pointer"
name="close-circle-line"
size="md"
/>
</span>
</>
) : null}
Comment on lines +278 to +305
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use semantic HTML elements for interactive controls

When creating interactive elements, it's important to use semantic HTML tags. Replacing <span> with <button> enhances accessibility and ensures that users can interact with your component using the keyboard.

Here's how you can update your code:

-                  <span
+                  <button
                     className="pl-1"
                     onClick={(e) => {
                       e.preventDefault();
                       e.stopPropagation();
                       onConfirm(value);
                     }}
                   >
                     <Icon className="cursor-pointer" name="check-line" size="md" />
-                  </span>
+                  </button>
-                  <span
+                  <button
                     className="px-1"
                     onClick={(e) => {
                       e.preventDefault();
                       e.stopPropagation();
                       onCancel();
                     }}
                   >
                     <Icon
                       className="cursor-pointer"
                       name="close-circle-line"
                       size="md"
                     />
-                  </span>
+                  </button>
📝 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
{value && !props.hideEditIcon && isEditing ? (
<>
<span
className="pl-1"
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
onConfirm(value);
}}
>
<Icon className="cursor-pointer" name="check-line" size="md" />
</span>
<span
className="px-1"
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
onCancel();
}}
>
<Icon
className="cursor-pointer"
name="close-circle-line"
size="md"
/>
</span>
</>
) : null}
{value && !props.hideEditIcon && isEditing ? (
<>
<button
className="pl-1"
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
onConfirm(value);
}}
>
<Icon className="cursor-pointer" name="check-line" size="md" />
</button>
<button
className="px-1"
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
onCancel();
}}
>
<Icon
className="cursor-pointer"
name="close-circle-line"
size="md"
/>
</button>
</>
) : null}


{savingState === SavingState.STARTED ? (
<Spinner size="md" />
) : value && !props.hideEditIcon && iconName ? (
Expand Down
5 changes: 1 addition & 4 deletions app/client/src/pages/Applications/ApplicationCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export function ApplicationCard(props: ApplicationCardProps) {
const [isDeleting, setIsDeleting] = useState(false);
const [isForkApplicationModalopen, setForkApplicationModalOpen] =
useState(false);
const [lastUpdatedValue, setLastUpdatedValue] = useState("");
const lastUpdatedValue = "";
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Class, let's examine this change in our state management.

The lastUpdatedValue has been transformed from a reactive state variable to a constant empty string. This simplification might affect how we handle updates to the application name.

Please consider the following:

  1. How will this impact real-time tracking of changes to the application name?
  2. Are there any other parts of the component that relied on lastUpdatedValue being reactive?

Let's discuss the reasoning behind this change and ensure it aligns with our learning objectives for this component.

const dispatch = useDispatch();

const applicationId = props.application?.id;
Expand Down Expand Up @@ -371,9 +371,6 @@ export function ApplicationCard(props: ApplicationCardProps) {
name: value,
});
}}
onTextChanged={(value: string) => {
setLastUpdatedValue(value);
}}
placeholder={"Edit text input"}
savingState={
isSavingName ? SavingState.STARTED : SavingState.NOT_STARTED
Expand Down