search

komodorio / helm-dashboard

The missing UI for Helm - visualize your releases
Apache License 2.0
5k stars 302 forks source link

UI - General - Shutdown modal #430

Closed KumarDhananjaya closed 1 year ago

KumarDhananjaya commented 1 year ago

Changes Proposed

Removed window close function and added a showdialog function to display the session end message on clicking the power button

Add all the screenshots which illustrate your changes

session.

undera commented 1 year ago

Why do you think the window close should be removed?

In any case, we're working now on a branch for V2 of UI: https://github.com/komodorio/helm-dashboard/tree/helm-dashboard-v2 , PRs like this should be done based on that branch.

And also your PR looks to contain changes to docs, irrelevant to the title of PR. You need to make sure your branch is up-to-date with the base branch.

KumarDhananjaya commented 1 year ago

I thought on pressing the power button we need to exit from the session with displaying this message. if we want to display this message and close the window then we can add timeout to this and close the window.

Regarding the changes in docs: i forgot to update this code in the local repo which raised a conflict.

can i still work on this with V2 of UI branch ?

undera commented 1 year ago

The power button is meant to close the window. The modal is displayed only for the case when it failed to close it, as a fallback. This is original intended behavior. I don't see sufficient explanation on why should we change that behavior. So I'm questioning overall need in this change.

Luckily, there are many other real issues in our project that you can implement, if you want to contribute.

KumarDhananjaya commented 1 year ago

According to my understanding previously, pressing the power button should've shown this message as exit, which was why the window close function was removed.

In this case, we've to close the window as the success of Ajax's request to shut down the application and as a fallback if the system fails to close the window we need to display this message.

Is my understanding of this behaviour correct?

undera commented 1 year ago

The logic is exactly: command to close the tab, then display the notice in case browser did not close the tab. IMO no change around it is needed for now.

KumarDhananjaya commented 1 year ago

Okay, i will work on other issues :)