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(modal): Hiding dimmer with multiple modals #2444

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

mareeo
Copy link
Contributor

@mareeo mareeo commented Aug 30, 2022

Description

This PR moves the check for other active/animating modals to hide the dimmer to the hide animation complete instead of the hide animation start.

Testcase

https://jsfiddle.net/xmd9eht6/

Screenshot

Original
Original

After change
Fix

Closes

#2441

…imation complete instead of animation start.
@lubber-de
Copy link
Member

Thanks for the PR, it basically works. However, hiding the dimmer after (even if just one) modal is hidden, is somehow disturbing the design/flow of the modal.
What if we just add the same code to onComplete (as you did but inside the if ( settings.allowMultiple ) { condition because this issue only happens on multiple modals, isn't it?) , but keep the code at onStart as well? Doesn't this also fix your issue?

…ultiple modal, also do dimmer check on hide animation complete.
@mareeo
Copy link
Contributor Author

mareeo commented Aug 30, 2022

Good catch, the my change caused the modal hide animation to start only once the modal was hidden. I've made the change you suggested and it does appear to work. If this edge case is reached, the dimmer will still start to hide once the modal is hidden, but I guess that's better than previous behavior of the dimmer not hiding at all.

One potential catch with this change, if you display a single modal with allowMultiple and then hide it, hideDimmer() is now called on the modal hide animation start and complete. This doesn't break anything, but it does result in the debug message "Animation is already occurring, will not execute repeated animation" if you have debug mode enabled.

I'd like to add a condition to prevent this, but I'd need some way to know if the dimmer is animating out as opposed to just simply animating (which is what it's doing now by calling the dimmer's is animating). I could check if the dimmer has the out class to determine if it's animating out and prevent duplicate calls in modal's hideDimmer().

Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

Please see my comment.
Regarding "avoid the debug info by checking for the 'out' class"
Well, the hideDimmer function does also support not using css classes but JS animation, so if someone possibly uses dimmerSettings: {useCss:false} checking for the 'out' class won't work.

The issue you mention actually comes from the "hideAll" function rather than your new change in the onComplete method.

So, instead, i suggest to check if "hide dimmer" is still in progress.
You can accomplish this by centrally adjusting the hideDimmer method as follows :

First: create new module wide variable isHiding

Snippet:

    time           = new Date().getTime(),
    performance    = [],
    isHiding       = false,

Second: Let the dimmers hide method return false in case the dimmer cannot be hidden

-> change to

return false;

module.debug('Dimmer is not visible');
}

-> change to

  module.debug('Dimmer is not visible'); 
  return false;
 }

Third: Adjust the hideDimmer method to check for that variable and take care of falsy return like this

Snippet

        hideDimmer: function() {
          if(!isHiding && ($dimmable.dimmer('is animating') || ($dimmable.dimmer('is active'))) ) {
            isHiding = true;
            module.unbind.scrollLock();
            if($dimmable.dimmer('hide', function() {
              isHiding = false;
              module.restore.bodyMargin();
              module.remove.clickaway();
              module.remove.screenHeight();
            }) === false) { isHiding = false;};
          }
          else {
             module.debug('Dimmer is not visible cannot hide');
            return;
          }
        },

Comment on lines +617 to +619
if(!module.others.active() && !module.others.animating() && !keepDimmed) {
module.hideDimmer();
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you please move this code where it was before (at the top of onStart), so this does not become an unnecessary change ? (Yes, logical wise it does not matter, but let's reduce changes to the minimum where needed 🙂 (also keeps the git history more clean)

@lubber-de lubber-de added type/bug Any issue which is a bug or PR which fixes a bug lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews state/awaiting-response Issues or pull requests waiting for a response labels Sep 2, 2022
@lubber-de lubber-de added this to the 2.9.x milestone Sep 2, 2022
@lubber-de
Copy link
Member

@mareeo Will you continue to work on this?

@mareeo
Copy link
Contributor Author

mareeo commented Sep 10, 2022

I plan to, just limited on time currently. Thanks for the detailed reply and suggestion, I'll give it a go once I have time

@lubber-de lubber-de added state/on-hold Issues and pull requests which are on hold for any reason and removed state/awaiting-reviews Pull requests which are waiting for reviews state/awaiting-response Issues or pull requests waiting for a response labels Sep 10, 2022
@lubber-de lubber-de marked this pull request as draft January 8, 2023 14:38
@lubber-de lubber-de removed the request for review from exoego January 8, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript state/on-hold Issues and pull requests which are on hold for any reason type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants