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

NiceModalProps should let objects with id #106

Closed osrl closed 9 months ago

osrl commented 1 year ago

NiceModalHocProps has a field named id. This is too common and prevents us to pass a type with id in it.

This is not possible:

NiceModal.create(
  (object: { id: string; foo: string }) => { ... }

I actually opened a PR for this #96. But closed it since it wasn't backward compatible.

To reproduce:

export const FoodModal = NiceModal.create(
  (arg: { id: string }) => {

    return (
      <div></div>
    );
  }
);

NiceModal.show(FoodModal, {id: "foo"}) // No overload matches this call
export const FoodModal = NiceModal.create(
  (arg: { fooId: string }) => {

    return (
      <div></div>
    );
  }
);

NiceModal.show(FoodModal, {fooId: "foo"}) // It's okay now
supnate commented 1 year ago

hmm, I remember calling modal.show(modal, args) can use any prop name.

supnate commented 1 year ago

Just tried and it works but with typescript error. Need to fix the type check of modal.show().


export const MyAntdModal = NiceModal.create(({ name , id}: { name: string, id: string }) => {
  const modal = useModal();
  return (
    <Modal title="Hello Antd" visible={modal.visible} onOk={modal.hide} onCancel={modal.hide} afterClose={modal.remove}>
      Greetings: {name} ({id})!
    </Modal>
  );
});

export default function AntdSample() {
  return (
    <Space>
      <Button type="primary" onClick={() => {
        //@ts-ignore
        NiceModal.show(MyAntdModal, { name: 'Nate', id: 'nateid' })}
      }>
        Show Modal
      </Button>
    </Space>
  );
}
czkoudy commented 10 months ago

Hi @supnate any luck with this?

supnate commented 9 months ago

Type check now fixed in v1.2.12. You can use id prop now.

chj-damon commented 7 months ago

@supnate I'm using 1.2.13 and still facing the same problem. typescript still reports error because I passed id prop as number type