search

humaan / Modaal

An accessible dialog window library for all humans.
http://humaan.com/modaal/
MIT License
2.72k stars 183 forks source link

add an open method. closes #63 #75

Closed james2doyle closed 6 years ago

james2doyle commented 8 years ago

This adds support to open the method. Similar to the close support.

For example:

$('.my-link').modaal('open');

I tested this on the demo page, and it seems to be working nicely.

I have a linter and formatter enabled on my editor. So there are some tweaks to the formatting.

Basically, I added a new method create_modaal which is used by both the element click-to-open and the new pass a string to close option. I also added code to stop the modal from being recreated when open is called on a modal that is already open.

james2doyle commented 7 years ago

any thoughts on this?

pzi commented 7 years ago

@james2doyle: any thoughts on this?

Personally, I feel that you should revert the changes your linter has made given that is a matter of personal preference and has nothing to do with the added functionality. Ultimately, it's up to the maintainer/project owner, though.

danhumaan commented 7 years ago

hi @james2doyle thanks for your contribution. It looks good and i'd like to do some testing on it to see how it all goes. Finding the time is a bit of a challenge at the moment so please bear with me while we balance the schedules.

With that said, I tend to agree with @pzi's comment, which I tend to agree with. There are a handful of committed updates that appear to be personal preferences or automated updates based on a linter. Granted there are some things that are highlighted as requiring attention (differences in indentation for example), but I would ask that you update your pull request so the changes recommended focus only on the new open method.

Cheers, Dan

james2doyle commented 7 years ago

I see what you mean. I can submit another PR but I am not sure when I will get around to it.

danhumaan commented 7 years ago

No worries @james2doyle - the other differences aren't going to stop us from doing some testing, so if you haven't submitted anything by then it's no trouble. If we're happy with how the new stuff is going we'll find a way to include it and address the other differences then.

Thanks again, looking forward to testing it soon.

james2doyle commented 7 years ago

Yeah, that works then. We already have this working on a site in production. We needed this feature so that we could open a specific modal on page load depending on a URL hash.

On Sun, Dec 11, 2016 at 4:46 PM Dan Moore notifications@github.com wrote:

No worries @james2doyle https://github.com/james2doyle - the other differences aren't going to stop us from doing some testing, so if you haven't submitted anything by then it's no trouble. If we're happy with how the new stuff is going we'll find a way to include it and address the other differences then.

Thanks again, looking forward to testing it soon.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/humaan/Modaal/pull/75#issuecomment-266321353, or mute the thread https://github.com/notifications/unsubscribe-auth/ABW_mPZhgF_qCjG6g5b2FfoE6j46jNnLks5rHJmAgaJpZM4KmTsw .

danhumaan commented 6 years ago

Thank you, this PR has been used as the basis for this implementation. Due to some other larger updates to the Modaal.js we've worked through this manually.

I'm going to close this PR but please keep an eye out for the next release for this implementation.

Cheers, Dan