birik / react-week-calendar

A flexible week calendar implemented in React.js.
MIT License
56 stars 35 forks source link

Add support for showing modal in edit vs create interval #5

Closed johnmryan closed 5 years ago

johnmryan commented 5 years ago

Hey there @birik! Thanks for this awesome module!

Our application would like to only show the modal for deleting intervals, and create interval would just automatically book the slot.

Not particularly happy with my implementation =/, but I tried to do it in a non-breaking change. Would appreciate any feedback if you have opinions on this.

If the approach looks reasonable to you, I will add corresponding tests (it passes the existing tests).

Note: I will also update the documentation and can add in an example of usage, just wanted to make sure we agree on an implementation first.

birik commented 5 years ago

Hi @johnmryan

Thank you for your commit. I think this is great idea to have this option. Can you implement a little bit another way:

What do you think about this solution? Is it valid for you?

Best regards, Evgeny

johnmryan commented 5 years ago

@birik Will do! I'll take some time to think about the naming for showModuleCase.

I should have an updated PR with the suggested changes by the weekend. Thanks!

johnmryan commented 5 years ago

@birik -- quick question, wanted some feedback.

If a user says:

useModal={false}
showModalCase=['create']

Should we hide the modal in this case, because they set useModal to false?

Might be helpful to consider the following examples:

useModal={true}
showModalCase=['edit'] // this will show modal for edit only?
useModal={false}
showModalCase=['edit', 'create'] // this will never show modal, since useModal is false

Thanks in advance!

johnmryan commented 5 years ago

If you agree that useModal: false should always hide the modal, can you give my code a quick look? That is the way I chose to implement it.

birik commented 5 years ago

@johnmryan Thanks for your commit. I will make small change and publish to npm

birik commented 5 years ago

@johnmryan The new version (0.1.3) with showModalCase was published to npm.