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

[v.17] Fix missing not saved warning #400

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

gevorgmansuryan
Copy link
Contributor

@gevorgmansuryan gevorgmansuryan requested a review from luke- July 2, 2024 20:22
Copy link
Contributor

@luke- luke- left a comment

Choose a reason for hiding this comment

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

@gevorgmansuryan Do we really need to implement a separate confirm message here?

Can't we trigger the existing Core Confirm dialogue somehow?

If the core dialogue does not allow this atm, a core update might also be useful here. This could potentially also occur in other modules.

@gevorgmansuryan
Copy link
Contributor Author

@luke- Core Confirm dialogue is browser thing, we do not control text. Current text is from core module, and used in many places.

@luke-
Copy link
Contributor

luke- commented Jul 5, 2024

@gevorgmansuryan My point is that I would like to use the existing code here:
https://github.com/humhub/humhub/blob/master/static/js/humhub/humhub.client.js#L429

It currently looks to me as if the mail module is implementing its own unload check.

Can't we find a common reusable solution here?

@gevorgmansuryan
Copy link
Contributor Author

gevorgmansuryan commented Jul 6, 2024

@luke- i can change it to use client.confirmUnload but for this we will need to update the core minversion of mail module, because confirmUnload method is not exported. Proceed this way?

@luke-
Copy link
Contributor

luke- commented Jul 23, 2024

@luke- i can change it to use client.confirmUnload but for this we will need to update the core minversion of mail module, because confirmUnload method is not exported. Proceed this way?

@gevorgmansuryan Yes please. It is not urgent and therefore a core solution would be preferable.

Copy link
Contributor

@luke- luke- left a comment

Choose a reason for hiding this comment

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

@gevorgmansuryan Thanks ,please see my remarks below.

widgets/ConversationView.php Outdated Show resolved Hide resolved
@gevorgmansuryan gevorgmansuryan requested a review from luke- July 26, 2024 03:39
@luke- luke- changed the title Fix missing not saved warning [v.17] Fix missing not saved warning Jul 30, 2024
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