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

Replace jquery.confirmExit #8264

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from

Conversation

onEXHovia
Copy link
Contributor

Subject

Parts #7158 #7156

Changelog

### Changed
Replace jquery.confirmExit to vanilla js

window.removeEventListener('beforeunload', this.onBeforeUnload);
}

// eslint-disable-next-line consistent-return
Copy link
Member

Choose a reason for hiding this comment

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

Isn't a code-smell to have a non consistent return ?

What should be fore if this.snapshotValue === this.snapshot ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return '' or return null or return false it may trigger dialog, of course if use retrun; will be work but eslint all the same throw error. We are already disable rule in https://github.com/sonata-project/SonataAdminBundle/blob/4.x/assets/js/jquery.confirmExit.js#L20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im mean it's will be not work, eslint all the same throw consistent-return error

  beforeUnload(event) {
    const message = Translation.trans('CONFIRM_EXIT');
    if (this.snapshotValue === this.snapshot) {
      return;
    }

    event.returnValue = message;
    return message;
  }

@VincentLanglet VincentLanglet requested review from a team, jordisala1991 and dmaicher February 25, 2025 14:47
@jordisala1991
Copy link
Member

Consider adding tests to this new controllers. It should be way easy to test this features with stimulus

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.

3 participants