18 - use dot based map cont - Man #71

ManSangSin · 2023-10-17T18:46:45Z

Naming Rules

Name your PR like this: ISSUENUMBER-TITLE-YOURNAME

Description

Set components structure to be Home -> MyMap -> DotMap -> RhythmCodeIcon
svg map now includes id for each dot
RhythmCodeIcon now use x and y data relative to the SVG map
x and y data is queried from the svg using the id
Removed toppx and leftpx / added map_id to rhythm_codes table
Update api query to match database changes
Update readme to include instructions on how to add a new marker

Related to

Make sure you include the issue number with a hash sign # so Github can link this PR to the right issue, like this:

Addresses #18

Checklist:

My code follows the style guidelines of this project
I have carefully reviewed my own code
I have commented my code
I have updated any documentation

ManSangSin · 2023-10-17T18:50:59Z

screenshot:

mferryRV

I'm not yet confident this is the right approach. I'd like to discuss two questions:

My understanding was that we would give each dot an ID and store the relevant ID in the database. What is the plan for transitioning away from PX-based positioning?
I would like to minimise the amount of interaction we have with DotMap.js. Is it possible to put DotMap inside MyMap? It was confusing to see MyMap imported but not referenced in the first 20,000 lines of the file.

mferryRV · 2023-10-17T19:15:09Z

client/src/components/DotMap.css

@@ -0,0 +1,38 @@
+/* dot map classes & ids */
+
+/* Cuba marker - can also use id #cuba */


Do we need these comments?

mferryRV · 2023-10-17T19:18:12Z

client/src/components/DotMap.js

This is a worringly large file, to the point where git is not able to display it for my review. Are there ways around editing this file?

How often does it need to be edited?

Can you explain the reasoning for putting the MyMap component inside the DotMap component vs vice versa?

mferryRV · 2023-10-17T19:19:21Z

server/db/build-rhythms.sql

@@ -29,10 +29,10 @@ CREATE TABLE rhythms (
    CONSTRAINT      rhy_rhythm_code_fk FOREIGN KEY (rhythm_code_id) REFERENCES rhythm_codes (id)
 );

-INSERT INTO rhythm_codes (rhythm_code, location, region, leftpx, toppx) VALUES('Rumba', 'Cuba', 'Caribbean', 570, 195);
-INSERT INTO rhythm_codes (rhythm_code, location, region, leftpx, toppx) VALUES('Gwo-Ka', 'Guadeloupe', 'Caribbean', 650, 180);
+INSERT INTO rhythm_codes (rhythm_code, location, region, leftpx, toppx) VALUES('Rumba', 'Cuba', 'Caribbean', 728.61, 742.22);


Are these still PX numbers?

mferryRV · 2023-10-17T19:19:47Z

client/src/pages/Home.css

+	cursor: pointer;
+}
+
+button {


Is this for all buttons? or is there a specific button?

mferryRV · 2023-10-17T19:20:25Z

client/src/pages/Home.css

+	background-color: rgb(255, 236, 167);
+}
+
+.menu > li > button {


I'm not sure this CSS belongs at the Home level - it seems like it might need to style a menu component. What is it styling?

ManSangSin · 2023-10-17T21:43:29Z

Add a readme to explain how to add a new location ID
structure: MyMap -> DotMap -> Children (location icons component)
rename leftpx toppx to match svg x / y in database

MyMap component is imported into DotMap

marker circles have radius of 20 and are plotted using centre of circle so offset is required to correct positioning

some dots were created using the ellipses shape, replaced with path equivalent to match rest of map

removed event listener

uses element id to find dot

map_id refers to the id of the dot inside the svg map

removed leftpx and toppx props

ManSangSin · 2023-10-18T11:43:27Z

client/src/components/DotMap.js

screenshot of code changes for DotMap component:

moved icon component into DotMap component as discussed last night

I've decided to add ids to the dots inside the svg as I think its cleaner and easier to explain when adding a new point to the map (via parsing the queryselector)

With this, you have to know what keys are being used from rhythmCodeObject - I'd prefer children

mferryRV

The query selector and IDs approach looks good 👍

I would prefer that RhythmCodeIcon stays in the MyMap component as a child of DotMap and we just pass children to the DotMap. It would mean nobody needs to look at or change DotMap in the future. With the current code, people would need to look deep into that SVG file to figure out what props we pass to RhythmCodeIcon. Basically I don't want anything imported or coded in DotMap.

This solution is acceptable (as we are nearing the end of the project), but I strongly recommend this change

mferryRV · 2023-10-18T17:58:34Z

client/src/components/RhythmCodeIcon.js

-
+	const dotSelector = document.querySelector(`#${map_id}`).outerHTML;
+	const findxy = /(?!=d="m)([0-9]*.[0-9]*),([0-9]*.[0-9]*)/;
+	const xyData = dotSelector.match(findxy);


I'm fine with this approach

ManSangSin requested a review from mferryRV October 17, 2023 18:46

mferryRV reviewed Oct 17, 2023

View reviewed changes

ManSangSin added 23 commits October 18, 2023 01:41

used semantic names for map component

a965bda

css selector for cuba dot

d3c7248

css selector for Guadelope dot

02eff2a

css selector for NOLA dot

bdb956f

css selector for Washington DC dot

9a26a06

unnecessary nesting of svg elements

7114f3a

better semantic name for class

9a7b7b2

set id for specific dots and tested click

c51ed16

group css selector style for markers

4922a5e

show mouse pointer on marker dot hover

1067d66

notes for id names

a01e91b

moved component styling to DotMap.css

bb59efd

made DotMap to be parent component

b348473

MyMap component is imported into DotMap

removed css styling of markers as unused

a82ee30

updated Rumba and Gwo-Ka leftpx and toppx values

c31f02d

set base x/y values to -10

ef72c59

marker circles have radius of 20 and are plotted using centre of circle so offset is required to correct positioning

set background and svg circle colour

6352ee6

added more / modified gradient colours

1378a6a

replaced ellipses with path elements in svg

3100fd2

some dots were created using the ellipses shape, replaced with path equivalent to match rest of map

removed redundant test function

4c46e32

removed event listener

set MyMap.js as the parent component

9d4e58a

imported RhythmCodeIcon into DotMap component

3a675a4

pass RhythmCodeIcon required data down as props

9c2c8f4

ManSangSin force-pushed the US18-use-dot-based-map-cont branch from 775273d to 9c2c8f4 Compare October 18, 2023 00:56

ManSangSin added 2 commits October 18, 2023 02:05

remove data from rebase

43011df

removed unnecessary comments

f12693b

ManSangSin added 9 commits October 18, 2023 11:04

updated svg with IDs for each path

533d95d

query selector for specific dot element inside map

41579ab

uses element id to find dot

defined a regex to find xy data from queryselector

8d33948

replaced toppx/leftpx with map_id

1351c16

map_id refers to the id of the dot inside the svg map

set data type for map_id to varchar

9c567de

replaced leftpx/toppx with map_id in query

ff7554a

passed map_id as prop to RhythmCodeIcon

b17ac54

removed leftpx and toppx props

replaced static query selector with prop

caf3351

used the xydata values for the icon positioning

60ccac8

ManSangSin commented Oct 18, 2023

View reviewed changes

ManSangSin added 2 commits October 18, 2023 12:46

removed console.logs

a373156

update readme to explain how to add new map marker

0c913c2

ManSangSin requested a review from mferryRV October 18, 2023 13:35

removed double row entry

0489949

mferryRV approved these changes Oct 18, 2023

View reviewed changes

ManSangSin added 2 commits October 18, 2023 19:53

passing RhythmCodeIcon via react children

0bc8807

added some comments

a069946

ManSangSin merged commit ffd22e8 into main Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

18 - use dot based map cont - Man #71

18 - use dot based map cont - Man #71

ManSangSin commented Oct 17, 2023 •

edited

Loading

ManSangSin commented Oct 17, 2023

mferryRV left a comment •

edited

Loading

mferryRV Oct 17, 2023

mferryRV Oct 17, 2023

mferryRV Oct 17, 2023

mferryRV Oct 17, 2023

mferryRV Oct 17, 2023 •

edited

Loading

mferryRV Oct 17, 2023

ManSangSin commented Oct 17, 2023

ManSangSin Oct 18, 2023 •

edited

Loading

mferryRV Oct 18, 2023

mferryRV left a comment

mferryRV Oct 18, 2023

		@@ -0,0 +1,38 @@
		/* dot map classes & ids */

		/* Cuba marker - can also use id #cuba */

18 - use dot based map cont - Man #71

18 - use dot based map cont - Man #71

Conversation

ManSangSin commented Oct 17, 2023 • edited Loading

Naming Rules

Description

Related to

Checklist:

ManSangSin commented Oct 17, 2023

mferryRV left a comment • edited Loading

Choose a reason for hiding this comment

mferryRV Oct 17, 2023

Choose a reason for hiding this comment

mferryRV Oct 17, 2023

Choose a reason for hiding this comment

mferryRV Oct 17, 2023

Choose a reason for hiding this comment

mferryRV Oct 17, 2023

Choose a reason for hiding this comment

mferryRV Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

mferryRV Oct 17, 2023

Choose a reason for hiding this comment

ManSangSin commented Oct 17, 2023

ManSangSin Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

mferryRV Oct 18, 2023

Choose a reason for hiding this comment

mferryRV left a comment

Choose a reason for hiding this comment

mferryRV Oct 18, 2023

Choose a reason for hiding this comment

ManSangSin commented Oct 17, 2023 •

edited

Loading

mferryRV left a comment •

edited

Loading

mferryRV Oct 17, 2023 •

edited

Loading

ManSangSin Oct 18, 2023 •

edited

Loading