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

feat(events): add close & dismiss events with preventDefault #158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paramburu
Copy link

Recently I needed a way to prevent a modal from closing. I could do a method in the scope but couldn't control backdrop or ESC key.

I want to introduce events with preventDefault capabilities so this kind of behavior can be controlled without much "hackery".

I propose naming these events, as you can see in the code, with the following structure:
`action:module:fndtn'

If you are interested please let me know so I can open an issue and propose which events could be beneficial and add them if you want.

@jbrowning
Copy link
Member

Hi @paramburu. Have you seen the backdrop and keyboard options for $modal.open()? From the docs:

  • backdrop - controls presence of a backdrop. Allowed values: true (default), false (no backdrop), 'static' - backdrop is present but modal window is not closed when clicking outside of the modal window.
  • keyboard - indicates whether the dialog should be closable by hitting the ESC key, defaults to true

Those two options seem to do exactly what you're trying to do.

@paramburu
Copy link
Author

Hi @jbrowning. Thanks for the response.

Indeed, I have seen them but they enable or disable the "closablility" of the modal on a global level. What I needed was to enable or disable it depending on validation of elements inside the scope.

With this change what I do inside the controller is the following:

$scope.$on('close:modal:fndtn', function (event) {
    if ($scope.importantValue > 100) {
        event.preventDefault();
    }
});

Maybe there is another way I couldn't find?

@jbrowning
Copy link
Member

I see. I don't think this is the best solution because you may encounter race conditions between the broadcasting of the events and the cancellation of the default behavior. As I see it, you have a couple of options:

  1. Disable backdrop and escape close and create your own listeners to perform your checks and close the modal when appropriate.
  2. Perhaps we could have an additional option on modal to perform this sort of behavior. Maybe a function to call when a close is attempted via backdrop click or escape. This function could be responsible for closing the modal when appropriate or ignoring the close attempt.

@jbrowning
Copy link
Member

Personally I think I'd rather go with 1. I don't think your use case is common enough to warrant this sort of behavior on $modal.

@paramburu
Copy link
Author

Thanks @jbrowning for the advice. I'm closing this PR then but still, I don't understand where the race condition might be. In theory every callback attached with $on will run before the if statement.

I believe it is useful to have events related with actions since it is a clean way to attach behavior. Even in the original foundation project there are events for almost all of the components.

@jbrowning
Copy link
Member

You are correct about the race condition concern. It appears that $broadcast will block until all listeners are called. I was mistaken. 😁

On second thought, your approach may actually work. ui-router takes a similar approach and broadcasts a $stateChangeStart event which may be canceled to prevent a state transition from happening.

I'd suggest simplifying the event name. Perhaps something as simple as fdnModalCloseStart and fdnModalCloseSuccess with similar names for dismiss?

@paramburu
Copy link
Author

Great! I will change the names and add other events as a proposal some time during this weekend. I saw someone that could benefit from this on issue #157.

Regarding the naming convention of angular events:
In jquery, event namespacing is made using dots, like 'eventname.namespace.othernamespace'. They handle events differently though and it would be confusing to use the same conventions. On the other hand, there isn't any naming convention on angular but as you stated they use the dollar sign followed by a camelCase name internally.

What I've seen on other projects is the ':' separator, like 'camelCaseName:nameSpace'. On this case I've used two namespaces because of the structure of the module but I want to suggest two more possible conventions:

  • modalClose:fdn & modalClosed:fdn
  • close:fdnModal & closed:fdnModal

I've used close & closed to mimic the original events (and for brevity) but could be closeStart and closeSuccess. Let me know what you think about them and if it will be merged if I manage to write the most useful events.

@BernardoMariano
Copy link

Is this going to be merged? I need broadcasted events just like this, but for dropdown-toggle.
Can I make a PR?

@terotil
Copy link

terotil commented Sep 27, 2016

@BernardoMariano Future of the project is to be decided, see #311

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