Wire up redirect from legacy map to new map #936
Conversation
jernestmyers
force-pushed
the
SAM-map-redirects
branch
from
0a4a5c7
to
01b7e40
Compare
|
I just started looking at this. I'm not seeing a modal, and it appears there are a couple of redirects going on. I see two POST requests to create an analysis, so I suspect that is why the modal isn't showing up. I will investigate more later. |
|
Hmm, it works for me. I'm using these urls while testing:
|
There was a problem hiding this comment.
This looks really great! See one suggestion I made below.
Also, this isn't directly related to your changes, but the fact that the side panel gets hidden automatically is less than ideal. It would be nice to see it when the modal is open, since the modal references it. I don't think anything needs to be changed in this branch, but I wanted to point it out, for future reference.
| const { analysisId } = await analysisClient.createAnalysis( | ||
| makeNewAnalysis(MEGA_STUDY_ID, computation, additionalConfig) | ||
| ); | ||
| setHasCreatedAnalysis(true); |
There was a problem hiding this comment.
I'm thinking this should be set before creating the analysis, so that the state us updated as soon as possible.
| @@ -115,6 +123,8 @@ export function MapAnalysis(props: Props) { | |||
| ); | |||
| const geoConfigs = useGeoConfig(useStudyEntities()); | |||
| const location = useLocation(); | |||
| const locationState = location.state as LegacyRedirectState; | |||
| const [showRedirectModal, setShowRedirectModal] = useState(!!locationState); | |||
There was a problem hiding this comment.
Sorry I looked at this yesterday and then had to do other things before I could comment.
Why don't we check that the contents of locationState are what is expected? Currently we're assuming that any history redirect with state metadata is a legacy redirect. It might not be in future?
There was a problem hiding this comment.
Seems like we should be checking for the presence and truthiness of locationState.showLegacyMapRedirectModal?
There was a problem hiding this comment.
I think you're right. I sort of glossed over this section. All the code right now is good, in that we aren't in danger of hitting TypeError: undefined errors.
There was a problem hiding this comment.
Maybe in the future it should be more like this, with some io-ts magic?
const locationState = location.state;
const [showRedirectModal, setShowRedirectModal] = useState(LegacyRedirectState.is(locationState) && locationState.showLegacyMapRedirectModal);
There was a problem hiding this comment.
I've addressed this feedback in a new PR: #983
Resolves #925