zakodium-oss / react-science

React components and tools to build scientific applications.
https://react-science.pages.dev
MIT License
2 stars 6 forks source link

Rewrite modal components using blueprintjss dialog #626

Closed wadjih-bencheikh18 closed 7 months ago

wadjih-bencheikh18 commented 8 months ago

Closes : https://github.com/zakodium-oss/react-science/issues/550 Closes: https://github.com/zakodium-oss/react-science/issues/623

cloudflare-pages[bot] commented 8 months ago

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2cacecb
Status: ✅  Deploy successful!
Preview URL: https://70ec44fc.react-science.pages.dev
Branch Preview URL: https://550-rewrite-modal-components.react-science.pages.dev

View logs

stropitek commented 8 months ago

Can you resolve the conflicts?

stropitek commented 8 months ago

In the complex content story, please try to make the content of the Dialog content overflow. When the text overflows, only the body part should scroll and there is a prop useOverflowScrollContainer on DialogBody that should help with this.

Actually this example is a bit too specific since the part that should scroll is not the body of the modal, it's a subpart of it. But I did not understand why overflow: visible is set on the modal.

wadjih-bencheikh18 commented 8 months ago

I think it's better to use https://blueprintjs.com/docs/#core/components/dialog.multistep-dialog for complexe dialog stories

stropitek commented 8 months ago

I think it's better to use https://blueprintjs.com/docs/#core/components/dialog.multistep-dialog for complexe dialog stories

This is only for dialogs that have sequential steps, which I don't think we have anywhere and which is not the case for the general configuration dialog that NMRium has.

I think we have to try to use the generic modal to do the configuration dialog.

wadjih-bencheikh18 commented 8 months ago

I think we have to try to use the generic modal to do the configuration dialog.

I tried to fix the layout, could you please take a look

stropitek commented 8 months ago

https://github.com/zakodium-oss/react-science/assets/4118690/12fd3eb6-52a3-4265-9bd2-f1381d563554

The vertical menu on the left should not scroll with the main content. Only the text should scroll.

stropitek commented 8 months ago

https://550-rewrite-modal-components.react-science.pages.dev/stories/?path=/story/components-modal--dynamically-sized-children

This story has a scrollbar, unlike all the other stories.

Otherwise LGTM

wadjih-bencheikh18 commented 8 months ago

@stropitek fixed

stropitek commented 8 months ago

I still have to scrollbar (edge / chrome)

CleanShot 2024-01-09 at 16 58 04

Can you rename the remaining stories that use the "Modal" term?

CleanShot 2024-01-09 at 16 59 09

wadjih-bencheikh18 commented 7 months ago

@stropitek fixed, Thank you