-
Notifications
You must be signed in to change notification settings - Fork 53
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
Leaflet Profile Occurrence Map #554
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, although I'm curious about the quotations (see comment) - I think I might stand to learn something there.
As noted in slack, there was some wonkiness with having to re-size the browser window to get the map to fully render on my local machine, but this is 1) not germane to the current PR and 2) likely a library issue.
collections/individual/index.php
Outdated
const marker = L.marker(mLatLng).addTo(map.mapLayer); | ||
} | ||
function initializeMap(){ | ||
if("<?php echo $LEAFLET?>") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about the quotes here. Why are they needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed to see if LeafLet has been activated for for the portal. Some portals might want to use LeafLet and others Google Map. However, this will produce a warning/notice in the error logs if the $LEAFLET variable hasn't been added to the symbini file, which will be most of the portals initially. Better to use !empty($LEAFLET), which will return true when variable exists and is true (e.g. true, 1, non-empty), and won't produce a warning if variable is not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. Mine was a syntax question. I was wondering why:
if(<?php echo $LEAFLET?>)
wasn't sufficient instead of if("<?php echo $LEAFLET?>")
with the doublequotes. But agreed about !empty
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, good point, it's due to it being a php boolan within a js conditional statement, which highlights that what I stated earlier doesn't work. You can't use !empty within a js condition. However, it will still throw a php warning whenever $LEAFLET is not declared within symbini. It might be better to convert the code to be totally to php within that section, maybe something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See note about us of !empty().
You have a mixture of tabs and 4 spaces being used for indenting code. Best to use one or the other within a page, or the indentation will look strange in other IDEs / interfaces. Since Symbiota has been using tab from the beginning, that is what we prefer.
Within the php code, best to use single quotes when string doesn't have to be evaluated for imbedded variables (e.g. "my $color cat"). Single quotes tell the processor that it can skip the evaluation step. This is very minor, thus not a big issue, but mentioning since it's listed in the code preferences.
…ent (#582) * create individualCollectionReorg.js file * bring new styling from css/v202209/symbiota/main.css over to css/uswds/symbiota/main.css * begin work on changing the order in collections/individual/index.php programmatically * add ids to a few more divs and fix an existing typo I think * finish div reordering via reorderElements method * Leaflet Profile Occurrence Map (#554) * Extracting google version, adding leaflet version * matching google and leaflet zooms * Fix for leaflet_tiles not loading * Changing Leaflet Check to be in php * Fixing tab/space issue * adding empty check to profile occurrence map * updating coord aids $LEAFLET CHECK * removing old coord aid $LEAFLET switch * Adding $LEAFLET improved leaflet check to clgmap * Leaflet public occurence map (#570) * Observation Leaflet Observation Icon * Leaflet-Occurence-Map * Added Leaflet Switch * 508 collections individual index (#578) * being to implement changes recommended by tvp * finish actionable total validator pro issues * improve spacing styling in collections/individual/index.css * fix typo found during self-review * add ids to a few more divs and fix an existing typo I think * add id to record-id-div and make code more readable * rename individualCollectionReorg.js to domManipulationUtils.js and add hr styling --------- Co-authored-by: Logan Wilt <[email protected]>
Pull Request Checklist:
Development
branch, NOTmaster
Development
andmaster
branches (at the same time).Development
branch, remember to use the squash & merge optionDevelopment
branch into the master branch, remember to use the merge optionDevelopment
branch to prevent accidental merges while QA takes placeThanks for contributing and keeping it clean!
Issue #537
Summary
Very simple map that just renders a marker where occurrence is located
Files Changed
Notes