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

Event Emission from viewmodel to Event Manager and Rename Project Flow Update #645

Closed
wants to merge 1 commit into from

Conversation

ibretsam
Copy link

Changes Summary:

  1. Event Emission from viewmodel.ts to Event Manager:

    • Events are now emitted from viewmodel.ts to the Event Manager. Since I’m still getting familiar with the codebase, I'm not entirely sure if this is the best approach yet, but it's a start. Let me know if my implementation looks correct or if there’s a better way to handle this.
  2. Rename Project Flow Update:

    • Added an onSubmit event to the NoBackgroundModal, which is now bound to the Enter key. This change allows users to submit the new project name by pressing Enter, while pressing ESC will cancel the renaming process.

    • Why do I do that? While testing the existing rename flow, I found it a bit confusing. Specifically, when the RenameProjectModal popped up, pressing ESC or clicking outside the modal would dismiss it, but the new project name would still be updated. Meanwhile, pressing Enter didn’t do anything at all. In most UI flows, ESC should cancel the action without making changes, and Enter should confirm it. That’s why I added the onSubmit method. Now, pressing Enter submits the new name and updates the project name, while ESC properly cancels without making any changes. This update should make the renaming process more intuitive and predictable.

@@ -7,6 +7,7 @@ dismiss the modal correctly.
<script lang="ts">
import { onMount } from 'svelte';

export let onSubmit: () => void = () => {};
Copy link
Author

Choose a reason for hiding this comment

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

I’ve initialized it as an empty function to ensure it's optional—other components using this one won’t need any changes, and everything will work as before if the function isn’t needed.

Copy link
Owner

@tuanchauict tuanchauict left a comment

Choose a reason for hiding this comment

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

Let me handle this task

return this._projectFlow.value!.find((item) => item.id === id);
}
}

export const projectDataViewModel = new ProjectDataViewModel();
const appContext = new AppContext();
export const projectDataViewModel = new ProjectDataViewModel(appContext.actionManager);
Copy link
Owner

Choose a reason for hiding this comment

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

This is not correct. appContext should be achieved from the context of Svelte.
Then we will set projectDataViewModel into the context for internal component used.

this.renamingProjectIdFlow.value = id;
this.actionManager.setOneTimeAction(OneTimeAction.ProjectAction.RenameCurrentProject(newName));
Copy link
Owner

Choose a reason for hiding this comment

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

This is not correct. setRenamingProject means set the target project for renaming, not setting the project name. Set project name is done by setProjectName

@@ -27,6 +28,8 @@ onMount(() => {
function onKeyDown(event: KeyboardEvent) {
if (event.key === 'Escape') {
dismiss();
} else if (event.key === 'Enter') {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we should handle this key inside the modal component container. This key should be captured explicitly by the content of the modal.

@tuanchauict tuanchauict closed this Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants