search

eBay / nice-modal-react

A modal state manager for React.
https://ebay.github.io/nice-modal-react
MIT License
1.96k stars 110 forks source link

fix: useModal show handle no longer making every prop optional and add NiceModa.show component props generic #70

Closed alexandredev3 closed 1 year ago

alexandredev3 commented 2 years ago

This PR addresses this other PR where it was discussed how to make the types more flexible, but apparently the pr was discontinued.

So, based on @jakst example I created this PR.

examples: missing_required_modal_props

required_props_prepared

I think the user should be able to pass a generic to say what props are expected. modal_show

Here I think the user should pass the required props because, as far as I'm aware, when we add the modal component directly in NiceModal.show(), we don't have to register the modal or anything. show_modal_component

supnate commented 1 year ago

Thanks @alexandredev3 , it looks like a nice solution. I will take a look later!

codecov-commenter commented 1 year ago

Codecov Report

Merging #70 (602677c) into main (6fc6562) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #70   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          186       186           
  Branches        29        29           
=========================================
  Hits           186       186
Impacted Files Coverage Δ
src/index.tsx 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

supnate commented 1 year ago

Hi @alexandredev3 , tried the PR locally, it behaves differently compared to the old modal.show api. For now, it seems always requires a type when create a modal: create<MyCompProps>(MyModal) instead of inferring type from MyModal. So I reverted the PR.

alexandredev3 commented 1 year ago

Hello @supnate, can you provide a demo of what you've tried? I tried to replicate what you've described, but it behaves as said before in this PR. Maybe I'm missing something 🤔.

supnate commented 1 year ago

Hi @alexandredev3 , sorry that I missed the last message. My bad, it's not caused by your pr. now I've recovered it and published in the new version v1.2.7.

duccadhv1 commented 1 year ago

how can i check required props when i use modal.show() with MyComponent name created by modal.create()?