georchestra / mapstore2-cadastrapp

repository for the mapstore2 version of cadastrapp
GNU General Public License v3.0
2 stars 10 forks source link

Fix modal container #176

Open arnaud-morvan opened 2 years ago

arnaud-morvan commented 2 years ago

MapStore2 Modal try to set it's container to ".ms2 > div": https://github.com/geosolutions-it/MapStore2/blob/5e4e0f5320550a648cb55994ebeea718d9915fcd/web/client/components/misc/WithContainer.jsx#L16

Note that it does not give a default but overwrite the value given to the constructor !

In MapStore2 this selector does not return any element, so container fallback on "body" as in react-bootstrap: https://react-bootstrap.github.io/react-overlays/api/Modal#container

When using some extensions (GeoNetwork, TJS), which inject tags <div class="ms2">..., modal does not appear anymore because those tags have "display: none". IMHO we should always inject modal in body, for a correct backDrop.

This fix it by using the standard modal from react-bootstrap.

arnaud-morvan commented 2 years ago

Hi @dsuren1

What do you think about removing withContainer from MapStore2:

https://github.com/geosolutions-it/MapStore2/blob/5e4e0f5320550a648cb55994ebeea718d9915fcd/web/client/components/misc/WithContainer.jsx#L16

Because:

Because here we also remove handleDialogClick which may have some useful effect:

https://github.com/geosolutions-it/MapStore2/blob/5e4e0f5320550a648cb55994ebeea718d9915fcd/web/client/components/misc/Modal.jsx#L24

But I do not see how to fix this use case without importing directly Modal from react-bootstrap

Or maybe we coult set it as a default value instead of an overwrite to avoid breaking something else.

Cheers

Arnaud