Skip to content
Snippets Groups Projects

feat(submaps tabs): open submap on click

Merged feat(submaps tabs): open submap on click
feature/MIN-123-open-submaps-from-list into development
4 unresolved threads
Merged Tadeusz Miesiąc requested to merge feature/MIN-123-open-submaps-from-list into development
4 unresolved threads

Description

In this PR I introduced changes to SubmapsDrawer and MapNavigation. Idea is to allow user switch easily between submaps.

Things done:

  • open submap from drawer
  • allow switch between the maps (tabs in top navigation)

More details about functionality

  • If map is not opened yet -> open it and set as currently active
  • If map is already opened -> set it as currently active
  • To set map as active it modifies current modelId in map reducer

Things not done

  • Maps don't currently remember last position set by user
Edited by Tadeusz Miesiąc

Merge request reports

Merge request pipeline #81126 passed

Stage: lint
    Stage: prettier
      Stage: test

        Merge request pipeline passed for 8292e62c

        Test coverage 92.97% (0.57%) from 1 job

        Merged by Tadeusz MiesiącTadeusz Miesiąc 1 year ago (Nov 9, 2023 10:39pm UTC)

        Loading

        Pipeline #81233 passed

        Stage: lint
          Stage: prettier
            Stage: test
              Stage: deploy

                Pipeline passed for ea558ff3 on development

                Test coverage 92.09% (0.57%) from 1 job

                Activity

                Filter activity
                • Approvals
                • Assignees & reviewers
                • Comments (from bots)
                • Comments (from users)
                • Commits & branches
                • Edits
                • Labels
                • Lock status
                • Mentions
                • Merge request status
                • Tracking
              • Adrian Orłów
                Adrian Orłów @AdrianOrlow started a thread on the diff
                • src/redux/map/map.reducers.ts
                20 27 // TODO to discuss manage state of failure
                21 28 });
                22 29 };
                30
                31 export const setActiveMapReducer = (state: MapState, action: SetActiveMapAction): void => {
                32 state.data.modelId = action.payload.modelId;
                33 };
                • Comment on lines +31 to +33
                  Adrian Orłów
                  Adrian Orłów @AdrianOrlow ·
                  Developer

                  Probably RFC

                  You're sure it is working correctly? I mean, is it changing the visible map and sets its center point and size?

                  As middleware is listening only for setMapData action, it shouldn't work in this case.

                  I propose leaving this solution and using setMapData action only.

                • Tadeusz Miesiąc
                  Tadeusz Miesiąc @teoMiesiac ·
                  Author Maintainer

                  Ok, I think we need to talk about how map works again. Thank you for spotting this out! From our conversation my understanding was that setting modelId would be enough and everything else will be automatic. Now I see other properties doesn't work as it should. Will make changes

                • Please register or sign in to reply
              • Adrian Orłów
                Adrian Orłów @AdrianOrlow started a thread on the diff
                • src/redux/map/map.reducers.ts
                21 28 });
                22 29 };
                30
                31 export const setActiveMapReducer = (state: MapState, action: SetActiveMapAction): void => {
                32 state.data.modelId = action.payload.modelId;
                33 };
                34
                35 export const openMapAndSetActiveReducer = (
                36 state: MapState,
                37 action: OpenMapAndSetActiveAction,
                38 ): void => {
                39 state.data.openedMaps.push({
                40 modelId: action.payload.modelId,
                41 modelName: action.payload.modelName,
                42 lastPosition: { x: 0, y: 0, z: 0 },
                43 });
              • Adrian Orłów
                Adrian Orłów @AdrianOrlow started a thread on the diff
                • src/redux/map/map.reducers.ts
                35 export const openMapAndSetActiveReducer = (
                36 state: MapState,
                37 action: OpenMapAndSetActiveAction,
                38 ): void => {
                39 state.data.openedMaps.push({
                40 modelId: action.payload.modelId,
                41 modelName: action.payload.modelName,
                42 lastPosition: { x: 0, y: 0, z: 0 },
                43 });
                44 state.data.modelId = action.payload.modelId;
                45 };
                46
                47 export const closeMapReducer = (state: MapState, action: CloseMapAction): void => {
                48 state.data.openedMaps = state.data.openedMaps.filter(
                49 openedMap => openedMap.modelId !== action.payload.modelId,
                50 );
                • Comment on lines +48 to +50
                  Adrian Orłów
                  Adrian Orłów @AdrianOrlow ·
                  Developer

                  Scenario below is an issue:

                  1. User opens map id=54 => it's added to openedMaps array AND modelId is set to 54

                  2. User closes map id=54 => it's removed from openedMaps BUT modelId is NOT changed

                  so in case of closing the map, it'll probably stay opened for the user. Or am I wrong?

                  This case is covered in closeMapAndSetMainMapActiveReducer but in this case - why there are two different reducers will completely different side effects?

                • Tadeusz Miesiąc
                  Tadeusz Miesiąc @teoMiesiac ·
                  Author Maintainer

                  I decided to move handling this logic to components: !47 (diffs) Imo making this kind of checks in reducer would only clutter it.

                • Please register or sign in to reply
              • Adrian Orłów
                Adrian Orłów @AdrianOrlow ·
                Developer

                LGTM but setActiveMapReducer is an RFC if I am not wrong

              • Adrian Orłów approved this merge request

                approved this merge request

              • Tadeusz Miesiąc added 1 commit

                added 1 commit

                • 4a1c589a - test(map navigation): separated two tests

                Compare with previous version

              • Tadeusz Miesiąc added 7 commits

                added 7 commits

                Compare with previous version

              • Adrian Orłów
                Adrian Orłów @AdrianOrlow started a thread on the diff
                • src/components/FunctionalArea/MapNavigation/MapNavigation.component.test.tsx 0 → 100644
                57 });
                58
                59 const mainMapButton = screen.getByRole('button', { name: 'Main map' });
                60 const mainMapCloseButton = await within(mainMapButton).queryByTestId('close-icon');
                61 expect(mainMapCloseButton).not.toBeInTheDocument();
                62
                63 const histamineMapButton = screen.getByRole('button', { name: 'Histamine signaling' });
                64 const histamineMapCloseButton = await within(histamineMapButton).getByTestId('close-icon');
                65 expect(histamineMapCloseButton).toBeInTheDocument();
                66
                67 const prknMapButton = screen.getByRole('button', { name: 'PRKN substrates' });
                68 const prknMapCloseButton = await within(prknMapButton).getByTestId('close-icon');
                69 expect(prknMapCloseButton).toBeInTheDocument();
                70 });
                71
                72 it('should close map tab when clicking on close button while', async () => {
              • Adrian Orłów
                Adrian Orłów @AdrianOrlow ·
                Developer

                LGTM

              • Adrian Orłów approved this merge request

                approved this merge request

              • Tadeusz Miesiąc mentioned in commit ea558ff3

                mentioned in commit ea558ff3

              • Tadeusz Miesiąc merged

                merged

                • Please register or sign in to reply