search

cheminfo / nmrium

React component to display and process nuclear magnetic resonance (NMR) spectra.
https://docs.nmrium.org
MIT License
55 stars 26 forks source link

Use Dialog from blueprintjs instead of `modal.show` #1537

Open lpatiny opened 2 years ago

lpatiny commented 2 years ago

@hamed-musallam Could you check if the modal.show can be replaced using the Analysis-ui-components ?

https://github.com/redhaam/analysis-ui-components/blob/ac8b11cd12dfb0b32e09e409760ca0bcb1be1007/src/layout/Modal.tsx#L15-L16

If there are any limitations we should improve analysis-ui-components.

hamed-musallam commented 2 years ago

@lpatiny

the current modal have limitation and I think it should be discussed with Michael, the modal in anaylsis-ui-compoennt is injected under RootLayout component and this will not work with NMRium because we need the modal deeply under all provider, and this is why we inject it at this level https://github.com/cheminfo/nmrium/blob/3865f572881a09d796d93de6a121b37c5d6d0ca3/src/component/NMRium.tsx#L362-L375

targos commented 2 years ago

I need a concrete example of something that doesn't work if the modal is injected elsewhere.

hamed-musallam commented 2 years ago

@targos

as I can see the modal is injected directly under the root layout component, does in the current version the modal can be injected elsewhere?

targos commented 2 years ago

No it cannot but I don't see what is the issue with it

hamed-musallam commented 2 years ago

in many cases, from any modal in NMrium we access the provider's hooks but the current implementation of the modal is not possible because it is on a different level in the tree. is this right?

targos commented 2 years ago

The only thing that matters is where the modal is rendered/open inside nmrium, not where the portal will be in the dom

targos commented 2 years ago

I'm going to migrate one modal in nmrium to show the new API

lpatiny commented 2 years ago

@hamed-musallam Michael is finishing the example and after you may refactor the code based on the example. For now there will be less options and the modal will always be centred on the screen and the background will always be disabled.

lpatiny commented 1 year ago

@wadjih-bencheikh18 Please make one PR per dialog !

wadjih-bencheikh18 commented 1 year ago

@targos Is it possible to use context hook to migrate modals? it just creating a context hook similar to the one we already have but using react-science Modal So we will be able to call useModal hook to use any modal easily

const modal=useModal()
modal.show(<ModalComponent {...props} />

the modal can be closes inside ModalComponent and we may do it using context.

Because right now I don't really like the way I'm migrating, we must have a button and it toggle the modal. And it's not always the case

https://github.com/cheminfo/nmrium/blob/aa3b45637fbe78781824bdce3f90efb4b53e07b1/src/component/1d/Viewer1D.tsx#L141-L150

Also the button needs to be in the modal which make the code less lisible and structured

https://github.com/cheminfo/nmrium/blob/aa3b45637fbe78781824bdce3f90efb4b53e07b1/src/component/modal/AboutPredictionModal.tsx#L51-L57

in the previous version the button is in the right place no like the next migrated example after migration where we have the modal in the place of the button https://github.com/cheminfo/nmrium/blob/aa3b45637fbe78781824bdce3f90efb4b53e07b1/src/component/panels/MoleculesPanel/MoleculePanelHeader.tsx#L221

targos commented 1 year ago

There's nothing in the Modal API that requires a button. You only need a state somewhere. The useModal is one of the biggest problems in NMRium, because it renders things that are possibly outdated by putting JSX elements in the state. We must move out of it.

wadjih-bencheikh18 commented 1 year ago

The useModal is one of the biggest problems in NMRium, because it renders things that are possibly outdated by putting JSX elements in the state. We must move out of it.

I understand now, thank you

targos commented 4 months ago

I renamed the issue to be more correct. We no longer have a modal component in react-science. The Dialog from blueprint is good.

wadjih-bencheikh18 commented 4 months ago

If one component can trigger more then one modal is it better to put in the component many modals each one with different toggle states or one modal and one toggle state and one content state?

For me the two ways of doing it are not clean. I was thinking maybe we can keep using the context but in diffrent way using blueprint dialogs

targos commented 4 months ago

Can you point to an example?

wadjih-bencheikh18 commented 4 months ago

This component have two modals for example There's also some other components like that

https://github.com/cheminfo/nmrium/blob/4f9c42010a5d09a5726b3262340d8afe61e0f64d/src/component/EventsTrackers/KeysListenerTracker.tsx#L202

targos commented 4 months ago

Do you have another example because this one is quite tricky and will require a big refactoring

wadjih-bencheikh18 commented 4 months ago

https://github.com/cheminfo/nmrium/blob/main/src%2Fcomponent%2Fpanels%2FRangesPanel%2FRangesHeader.tsx#L102

https://github.com/cheminfo/nmrium/blob/main/src%2Fcomponent%2Fpanels%2FZonesPanel%2FZonesPanel.tsx#L113

https://github.com/cheminfo/nmrium/blob/main/src%2Fcomponent%2Fpanels%2FSummaryPanel%2FCorrelationTable%2FCorrelationTableRow.tsx#L200

targos commented 4 months ago

The problem with the current way it's done (context), is that it's not reactive. If something is changed in the React state while the Modal is open, the contents (or behavior) can be wrong because it's not updated. I agree that doing many toggle states in the same component is suboptimal, though. Don't hesitate to suggest higher-level changes (for example to the DefaultPanelHeader component) to make it easier to handle modals.