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

[fix] Unsaved changes alert disable on preview view #857

Merged
merged 5 commits into from
May 8, 2024

Conversation

Dhanus3133
Copy link
Member

Fixes #824

@coveralls
Copy link

coveralls commented May 6, 2024

Coverage Status

coverage: 98.75%. first build
when pulling c66e0fa on Dhanus3133:bug/unsaved-changes-preview
into ca9e83c on openwisp:master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

There's a few issues with this solution:

  1. A user may have done unsaved changes, hit the preview and then close the window by mistake, the unsaved changes logic is disabled and the window will be closed, which is not what we want.
  2. Opening and closing the preview can be done also with keyboard shortcuts, opening the preview with ALT and P does not trigger this code.

Rather than a solution, to me this feels like hiding dust under the carpet: the dust will just keep increasing until it's too much. Please let's refrain from this kind of solution and let's debug the code.

if you add these at line 52-53:

console.log(initialValue);
console.log(currentValues[name]);

The console log shows that the initial value is null, while after the preview the value is and empty string, the two values do not match.

Therefore, we must investigate how it works and understand why null changes to empty string after the preview, with that information we can look for possible solutions, for example:

  • maybe whatever is changing the value from null to empty string shouldn't happen
  • or maybe when comparing values of Select2 elements, we can consider null and empty string to be the same

Which one of the two is right? I think it's premature to decide without further information.

@Dhanus3133 Dhanus3133 force-pushed the bug/unsaved-changes-preview branch from 939bf95 to bc45b3b Compare May 8, 2024 16:03
@Dhanus3133
Copy link
Member Author

@nemesifier Updated with the changes. Kindly review it again!

@nemesifier nemesifier merged commit f0bdf03 into openwisp:master May 8, 2024
12 checks passed
@Dhanus3133 Dhanus3133 deleted the bug/unsaved-changes-preview branch May 9, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[bug] Unsaved changes alert shown for templates and VPN with shared organization
3 participants