fix: empty space is clickable when user edits app name or organisatio… #36882
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
|
Hi @riteshkew, As per our discussion i made changes, can you please review it and let me know if any changes required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
app/client/src/pages/Applications/ApplicationCard.tsx (3)
Line range hint
290-305: Let's turn our attention to thehandleMenuOnClosefunction, class.We've made some adjustments to how we update the application name. Now, we only update if
lastUpdatedValueis not empty and the name has actually changed.Consider the following points for our discussion:
- How does this change affect the user experience when editing the application name?
- Are we ensuring that all valid name changes are captured and saved?
- Could we improve error handling or user feedback in this function?
Remember, clear and intuitive user interactions are key to a good learning experience. Let's make sure our changes support this goal.
Line range hint
290-305: Class, let's examine an important change in our component's behavior.We've removed the
onTextChangedevent handler for theEditableTextcomponent. This means we're no longer tracking changes to the application name in real-time.Let's consider the following questions:
- How does this affect the user's ability to see their changes as they type?
- Are we still providing adequate feedback to the user about the status of their edits?
- Could this change lead to any confusion or frustration for users who expect immediate feedback?
Remember, in our classroom of code, we always strive for clarity and responsiveness. Let's discuss if this change aligns with those principles or if we need to revisit our approach.
Line range hint
1-605: Class, let's summarize our review of the ApplicationCard component.We've observed several significant changes:
- The simplification of
lastUpdatedValuestate management.- Modifications to the
handleMenuOnClosefunction.- Removal of real-time tracking for application name changes.
For our next steps, I suggest:
- Conducting thorough user testing to ensure these changes don't negatively impact the user experience.
- Reviewing our error handling and user feedback mechanisms in light of these changes.
- Considering if we need to add any additional safeguards or confirmations for name changes.
Remember, every line of code is an opportunity to learn and improve. Let's take these insights and use them to make our application even better in our next iteration.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/packages/design-system/ads-old/src/EditableTextSubComponent/index.tsx (5 hunks)
- app/client/src/pages/Applications/ApplicationCard.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
app/client/packages/design-system/ads-old/src/EditableTextSubComponent/index.tsx
[error] 232-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
app/client/packages/design-system/ads-old/src/EditableTextSubComponent/index.tsx (2)
141-141: Good addition of state variableisCancelledIntroducing
const [isCancelled, setIsCancelled] = useState<boolean>(false);is a thoughtful approach to manage the cancellation state in the component.
191-192: Ensure state updates are in syncUpdating
setLastValidValue(finalVal);andsetValue(finalVal);upon confirmation is a good approach to keep your component's state consistent.
| @@ -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 = ""; | |||
There was a problem hiding this comment.
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:
- How will this impact real-time tracking of changes to the application name?
- Are there any other parts of the component that relied on
lastUpdatedValuebeing reactive?
Let's discuss the reasoning behind this change and ensure it aligns with our learning objectives for this component.
| @@ -70,6 +70,7 @@ const TextContainer = styled.div<{ | |||
| }>` | |||
| display: flex; | |||
| align-items: center; | |||
| // justify-content: space-evenly; | |||
There was a problem hiding this comment.
🛠️ 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.
| const onCancel = useCallback(() => { | ||
| onBlur && onBlur(lastValidValue); | ||
| setIsEditing(false); | ||
| setIsInvalid(false); | ||
| setSavingState(SavingState.NOT_STARTED); | ||
| setValue(lastValidValue); | ||
| setIsCancelled(true); | ||
| }, [lastValidValue, onBlur]); |
There was a problem hiding this comment.
🛠️ 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.
| 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]); | ||
|
|
There was a problem hiding this comment.
🛠️ 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.
| useEffect(() => { | |
| if (isCancelled) { | |
| setValue(lastValidValue); | |
| } | |
| }, [isCancelled, lastValidValue]); | |
| const onCancel = useCallback(() => { | |
| onBlur?.(lastValidValue); | |
| setIsEditing(false); | |
| setIsInvalid(false); | |
| setSavingState(SavingState.NOT_STARTED); | |
| setValue(lastValidValue); | |
| }, [lastValidValue, onBlur]); |
| {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} |
There was a problem hiding this comment.
🛠️ 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.
| {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} |
|
@Sai6347 The following things doesn't seem to work correct after these changes:
4. Even the widget name editor on the property pane isn't working:
|
|
@Sai6347 We are currently working to bring consistency across all components used in the product, and we’ll inform you once those components are ready for use. Thank you for your efforts, and please feel free to contribute to other issues in the meantime. |
|
@ankitakinger |
Bug issue
Description:
In the home page when the user clicks the edit(pencil) icon to edit the application or organization name, edit icon is hiding and if we click on empty space where the edit icon is initially placed, we are able to save the name. Now i have implemented Save(Tick) and Cancel(Cross) icons, if we click on Save icon it will save or click on Cancel icon to cancel the editing.
Initially, any changes to the edited data would automatically save and display the updated application name when the user clicked anywhere. However, I have modified the functionality so that the name only changes when the user explicitly clicks the "Save" (tick) icon. Otherwise, the old name will remain displayed.
Screenshots
Before the fix:
After the fix:
Summary by CodeRabbit
EditableTextSubComponentwith clear cancel and confirm actions.ApplicationCardcomponent for better performance and reduced unnecessary re-renders.