cheminfo
Use Dialog from blueprintjs instead of `modal.show`
@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.
@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
I need a concrete example of something that doesn't work if the modal is injected elsewhere.
@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?
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?
The only thing that matters is where the modal is rendered/open inside nmrium, not where the portal will be in the dom
@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.
@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
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.
The
useModalis 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
I renamed the issue to be more correct. We no longer have a modal component in react-science. The Dialog from blueprint is good.
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
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
Do you have another example because this one is quite tricky and will require a big refactoring
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
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.