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

Add fallback timeout to reset modalClosing flag in ModalManager #3929

Closed
wants to merge 1 commit into from
Closed

Add fallback timeout to reset modalClosing flag in ModalManager #3929

wants to merge 1 commit into from

Conversation

iPurpl3x
Copy link
Contributor

Changes proposed in this pull request:

  • Added a fallback timeout to the animateHide function to ensure the modalClosing flag is reset. This change aims to prevent potential issues where a modal could become unresponsive if the modalClosing flag remains set to true.

Reviewers should focus on:

  • The implementation of the timeout in the animateHide function.
  • Ensuring that the addition of the timeout and the clearTimeout call do not introduce any regressions or unintended side effects.

QA

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

@iPurpl3x iPurpl3x requested a review from a team as a code owner November 16, 2023 09:47
@SychO9 SychO9 deleted the branch flarum:1.x January 10, 2024 15:29
@SychO9 SychO9 closed this Jan 10, 2024
@iPurpl3x
Copy link
Contributor Author

@SychO9 can you explain why this got closed?

@imorland
Copy link
Member

@iPurpl3x I would guess because it's targetting 1.x? Other than fixes, 1.x is more or less in feature freeze. I'm sure if you could retarget for 2.x we could accept this change 🤔

I'll wait to see what @SychO9 thinks also...

@SychO9
Copy link
Member

SychO9 commented Jan 16, 2024

I deleted the 1.x branch to replace it with another to cleanup a few commits when working on the extension manager which is what seems to have auto closed this on delete.

reopening but we are pretty much only patching critical issues for 1.x to focus on 2.x development. We could include this in a patch should anything else (or more fixes) turn up for core as well but we unfortunately don't intend to make a new patch for every 1.x PR 🙏🏼, I hope you understand that just wouldn't be feasible.

@SychO9 SychO9 reopened this Jan 16, 2024
@glowingblue glowingblue closed this by deleting the head repository Jan 27, 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.

4 participants