cyclejs-community / cyclejs-modal Goto Github PK

breaking change suggestion..

modify modalify ( pun intended ) arguments to use object instead of a list

modalify( app, {
  name : 'modal',
  center : true,
  modalContainerClass : 'some-extra-class',
  background : 'rgba(0,0,0,0.8)',
  zIndex : 500
})

it reads better than

modalify( app, 'modal', true, 'why-this-string-here', 'rgba(0,0,0,0.6)', 500 )

you don't know what modal does. what true is.. and what 500 is for.. and the random string

It looks like various Cycle changes have left cyclejs-modal behind. I get errors like this trying to compile an app that uses cyclejs-modal:

[at-loader] ./node_modules/cyclejs-modal/node_modules/@cycle/dom/lib/MainDOMSource.d.ts:6:22 
    TS2420: Class 'MainDOMSource' incorrectly implements interface 'DOMSource'.
  Types of property 'select' are incompatible.
    Type '(selector: string) => DOMSource' is not assignable to type '<S extends DOMSource>(selector: string) => S'.
      Type 'DOMSource' is not assignable to type 'S'. 

With a hint in the right direction I could attempt a PR; or if someone is actively maintaining cyclejs-modal a keep-up-with-core-Cycle release would be appreciated.

currently, isolation is handled by modalify internally.

if we want to use onionify with custom lenses we can't.

proposal is to remove isolation and let the user pass in an already isolated component on their own in the open action stream.

thoughts? @jvanbruegge

this suggestion is a breaking change tho..

When using cyclejs-modal in an application which some elements (navigation, etc) that have a non-default z-index, the opaque background fails to cover those elements and leave them still operable.

The most important, for my use case, would be to have the modalContainerClass option used even with the center: false option.

This way I would be able to handle all the positioning in my CSS files without having to manually wrap all my modals in a <div class="my-modal-wrapper">

I believe it is just a matter of consuming the modalContainerClass option here

I'd be happy to create a PR

function main({ DOM } : Sources) : Sinks
{
    return {
        DOM: Rx.Observable.of(button('.button', ['open modal'])),
        modal: DOM.select('.button').events('click')
            .mapTo({
                type: 'open',
                component: modal
            } as ModalAction)
    };
}

function modal({ DOM } : Sources) : Sinks
{
    return {
        DOM: Rx.Observable.of(div('.div', [
            span('.span', ['This is a modal. Yeah? :)']),
            button('.button', ['close'])
        ])),
        modal: DOM.select('.button').events('click')
            .mapTo({ type: 'close' } as ModalAction)
    };
}

this would break as of the example.. there isn't enough conversion of the sinks..

replacing this with

const parentSinks : Sinks = main({
  ...sources, [name]: adapt(messageProxy$)
});

const sinks : Sinks = Object.keys(parentSinks)
  .map(k => ({ [k]: xs.fromObservable(parentSinks[k]) }))
  .reduce(( prev, curr ) => Object.assign( prev, curr ), {});

should do the magic

Is this working atm? I get error when I run "npm i && npm run examples":

Error: ENOENT: no such file or directory, open 'examples/simple/build/index.js'
at Error (native)

Currently there's no way of us to get the click event of the overlay.

I'm not sure how the implementation would look like but it would be nice if we are able to listen to clicks on the overlay and then react upon it ( like closing the modal when they click the overlay )

Attached log
2017-05-19T08_17_35_365Z-debug.txt