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

show districts name, population & other relevant information on popup when hovered upon district #29

Closed
wants to merge 24 commits into from

Conversation

mrsudarshanrai
Copy link

@mrsudarshanrai mrsudarshanrai commented Feb 17, 2022

Added feature issued here.
Added some more data about district like district Area, Population & Elevation. This data will be visible upon mouse hover on districts.

@vercel
Copy link

vercel bot commented Feb 17, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/aabishkaryal/nepal-districts/GXxV68sXrkb7NoRouan1p5c8fkxv
✅ Preview: https://nepal-districts-git-fork-mrsudarshanrai-main-aabishkaryal.vercel.app

@aabishkaryal
Copy link
Owner

@mrsudarshanrai please follow the naming convention for components
just Modal.tsx is good

@aabishkaryal
Copy link
Owner

do we really need the additional districtArea on data? It is possible to add km2 while rendering.

@aabishkaryal
Copy link
Owner

Also, please PR into main, sorry for the trouble caused due to this.

Comment on lines 13 to 16
const Modal = (props: props) => {
const { data } = props || {}
const { pageY, pageX, target } = data || {}
const district = DistrictData[(target as any)?.id]
Copy link
Owner

Choose a reason for hiding this comment

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

If you're using just pageX, pageY and target from the event, why don't you just pass them as props instead of passing the whole event?

Copy link
Owner

Choose a reason for hiding this comment

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

Also take a look at how other components are declared.
Let's keep it consistent.

import DistrictData from '../../data/districts.json';

type props = {
data: React.MouseEvent<SVGPathElement> | null;
Copy link
Owner

Choose a reason for hiding this comment

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

It is never null, you are checking that before rendering the modal component.

Comment on lines 7 to 8
onMouseOver: (event: React.MouseEvent<SVGPathElement>) => void
onMouseOut: (event: React.MouseEvent<SVGPathElement>) => void
Copy link
Owner

Choose a reason for hiding this comment

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

These need to be optional.

<svg viewBox="0 0 1026.077 519.136" className="w-full">
<g transform="translate(-52.379 -15.971)">
{visibleDistrictIndices.map((index) => (
<District {...districts[index]} key={districts[index].zip} />
<District id={index} {...districts[index]} key={districts[index].zip} onMouseOver={(event) => setModalState(event)} onMouseOut={() => setModalState(null)} />
Copy link
Owner

Choose a reason for hiding this comment

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

can be replaced with

onMouseOver={setModalState}

@mrsudarshanrai
Copy link
Author

do we really need the additional districtArea on data? It is possible to add km2 while rendering.

Oh my my, I didn't realize whole entire time that area === districtArea , Ok will use the existing area.

@mrsudarshanrai
Copy link
Author

@aabishkaryal So I am using the existing area value now. For now, values are not comma-separated. (eg,2322 km2) Do we need them comma-separated?(eg 2,333 km2) Other factors such as population and elevation are comma-separate.

@aabishkaryal
Copy link
Owner

@aabishkaryal So I am using the existing area value now. For now, values are not comma-separated. (eg,2322 km2) Do we need them comma-separated?(eg 2,333 km2) Other factors such as population and elevation are comma-separate.

You could use toLocaleString with "en-US". Read the documentation for more details.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toLocaleString

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.

None yet

4 participants