Feature/improve config

[kevcjones-archived]

PR to help with code review. Will merge when we're happy with a final set of configurations and the implementation.

[kevcjones-archived]

This PR tackles both #10 and #7

[celsomtrindade]

I just remembered the original project had the option to define the container for the modal when importing the module

BootstrapModalModule.forRoot({ container: document.body });

Did you removed this option, right? I didn't saw this anymore, but I think this is the way to go, default to document.body. Just to confirm this change.

[kevcjones-archived]

Ah sorry putting the description here https://github.com/KevCJones/ngx-simple-modal/issues/7#issuecomment-348142965 probably not the best place. Re: Name changes

I like them - will implement.

Re: wrapperClass

Tbh my rationale was more about classname conflict avoidance rather than improvement, but its also a chance to write your own animation class, which you could argue can still happen by overriding our chosen class. Longer term i had mind to think about letting them completely customise the show and hide animations by configuration.

wrapperClass: { onShow: 'in', onHide: 'out'}

Just thinking out loud.

In terms of conflicting styles, one strategy would be to pick class names or rules which are unlikely to be polluted from global rules... if you have suggestions for names i'm happy to remove another config?

Re : hideOthers is a TODO but i think i'll hide it for the moment because its a big TODO after some thought... in the 90% use case i don't think stacking modal backgrounds bothers anybody... maybe just me?

Re : autoCloseTimeout

Well modals can contain dialogs, its still a fuzzy area for me, but you have a point, there is a question of responsibility and whether timeouts are out of scope for something that is purely for managing. I mean there isn't anything here that couldn't be a simple setTimeout inside of your implementation that calls close and is placed in something like ViewAfterInit. So by the line of thinking, its just a nice to have.. and i'm quite happy to bin it, its one less test i'll need to write.

[celsomtrindade]

Re: hideOthers

In this case, maybe a new issue to discuss this topic? I'm currently having to make a manual work around to fix some "issues" on this.

Re: wrapperClass

Since you mentioned the problem with global styles on the css (avoid conflict of class .in, wich has a low level of specificity) the only solution I can think of is to use a BEM like css class name, with a self prefix.

.sm_modal.sm_modal-in { }
.sm_modal.sm_modal-out { }

This way users are free to animate them the way they prefer. It's still easier than create a css trigger animation, because just have to set the default open style and the style to animate from, since the code will handle the add/remove class.

Other than that, your proposition is the way to go, otherwise it may have class name conflicts.

Note: I'm just trying to think things that may be easier to maintain on the css side instead of JS :)

[celsomtrindade]

About the wrapperClass, I made a few simple tests and maybe we can move the wrapperClass and animationDuration out of the JS code and use it with css only, using a keyframes css configuration. See this example. This can remove some complexity from the code and add more possibilities to animate the modal.

[kevcjones-archived]

I think i'd have to see how you plan to make it work for the hide animation too to understand how you will solve this flow with only css https://github.com/KevCJones/ngx-simple-modal/blob/feature/improve-config/src/simple-modal-holder.component.ts#L79 when removing, do some DOM stuff, like class removals, then wait, then remove the element from the DOM.

Fresh eyes, and i wasn't happy with the readability of the code and also i foresaw a headache with where i'd put the core 'remove' logic. So did some cleaning up. Oh and removed the configs we agreed we're not needed.

[kevcjones-archived]

I'm going to leave this as is unless you see anything pressing. You are going to modify at CSS level anyway so can unblock you while you get things going

[celsomtrindade]

Hum.. maybe you're right.. The remove part isn't working as expected. I could make it work, kind of.. But not with consistency. Will have to look into it with more time.

For now, I think the PR is good to go!

[kevcjones-archived]

Cool , yeah the animation complete API's are flakey at best. Been down that, ends up with hardcoded fallbacks anyway in my experience. Merging in now.

[kevcjones-archived]

I don't think I did, that's a config you define at module level. I can think it defaults as a child of root app, I actually nest mine deeper in personal project for some theming inheritance ( styles) ...

My only change to this was I made it promise supporting so that you can define a container which might not exist at startup without a crude workaround.

No qualms in defaulting to document.body if there is a positive reason? Letting it create the holder inside your root component seems like the more obvious default - namely because it's inside angulars native domain.

On 30 Nov 2017 15:20, "Celso Trindade" notifications@github.com wrote:

I just remembered the original project had the option to define the container for the modal when importing the module

BootstrapModalModule.forRoot({ container: document.body });

Did you removed this option, right? I didn't saw this anymore, but I think this is the way to go, default to document.body. Just to confirm this change.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/KevCJones/ngx-simple-modal/pull/11#issuecomment-348219525, or mute the thread https://github.com/notifications/unsubscribe-auth/AAqe8PFSZ-ovw44SYVlGoXED6IeVkM05ks5s7seygaJpZM4QwODE .