Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Waitp 1224 overlay selector desktop #4685

Merged
merged 7 commits into from
Jun 28, 2023

Conversation

alexd-bes
Copy link
Contributor

Issue WAITP-1224: Map overlay selector on desktop - part 1

Changes:

  • Updated DesktopMapOverlaySelector to be fully functional
  • Added MapOverlayList for use across mobile and desktop overlay selectors
  • Added new useMapOverlays hook to utils that will also return useful map overlay associated info

Screenshots:

No overlays available
Screenshot 2023-06-28 at 2 44 23 pm

Overlays available and selected
Screenshot 2023-06-28 at 2 45 21 pm

import { get } from '../api';
import { EntityCode, ProjectCode } from '../../types';

export const useFetchMapOverlays = (projectCode?: ProjectCode, entityCode?: EntityCode) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure what to call this query hook, but I have another hook in utils called useMapOverlays which means I can't call this the same thing

Copy link
Contributor

@tcaiger tcaiger Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to just move all the code from packages/tupaia-web/src/utils/useMapOverlays.ts into this file and name this one useMapOverlays since it is just derived api data with the exception of updateSelectedMapOverlay. You could then put updateSelectedMapOverlay inline in MapOverlayList, make it a stand alone util or keep it with useMapOverlays if you really want.

['mapOverlayData', projectCode, entityCode, mapOverlayCode],
async () => {
return get(
`measureData?mapOverlayCode=${mapOverlayCode}&organisationUnitCode=${entityCode}&projectCode=${projectCode}&shouldShowAllParentCountryResults=${
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be great to use the axios params field here for readability and ease of maintenance.
Also Ethan has re-done the endpoints for the map overlays so this should use legacyMapOverlayReport or report depending on whether or not it is legacy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that endpoint for fetching the measure data, or for something else?

Copy link
Contributor

@tcaiger tcaiger Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that's right for measureData

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm currently getting an error using this, and it seems to be different to measureData which is for map overlays (is that right?) and reports which I was using for dashboard report data. I'll add this in as part of part 2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah it looks like there is a bug in the endpoint so yes that's leave this as is for now

@media screen and (max-width: ${MOBILE_BREAKPOINT}) {
display: none;
}
`;

const Legend = styled.div`
height: 50px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should be rems, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, but this is just a placeholder so I don't think it's important at this stage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh true. I just see px and I jump 😆

import { Entity } from '../../../types';
import { useMapOverlayData } from '../../../api/queries';
import { useParams } from 'react-router';
import { periodToMoment } from '@tupaia/utils';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just check order of imports here

import { URL_SEARCH_PARAMS } from '../constants';
import { MapOverlayGroup, SingleMapOverlayItem } from '../types';

const flattenMapOverlays = (mapOverlayGroups: MapOverlayGroup[] = []): SingleMapOverlayItem[] => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this mapOverlaysByCode and have it generate a dictionary of the map overlays like below. Then we could just lookup the result below instead of finding it. eg mapOverlaysByCode[selectedOverlayCode]

{ 
  123: {...},
  128: {...}
}

Copy link
Contributor

@tcaiger tcaiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. I left a couple of suggestions. Happy to discuss anything if you like.

@alexd-bes alexd-bes requested a review from tcaiger June 28, 2023 04:18
@alexd-bes alexd-bes merged commit d18b38f into epic-frontend-rewrite Jun 28, 2023
@alexd-bes alexd-bes deleted the waitp-1224-overlay-selector-desktop branch June 28, 2023 04:56
tcaiger pushed a commit that referenced this pull request Jul 12, 2023
* WAITP-1224 initial working selector

* WAITP-1244 Tidy up files

* Update border radius

* Update border radius logic

* Tidy ups
tcaiger pushed a commit that referenced this pull request Jul 12, 2023
* WAITP-1224 initial working selector

* WAITP-1244 Tidy up files

* Update border radius

* Update border radius logic

* Tidy ups
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants