-
Notifications
You must be signed in to change notification settings - Fork 0
Group 1 Review #1
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
base: jan2025-code-review/merge-branch
Are you sure you want to change the base?
Group 1 Review #1
Conversation
|
||
import colorbrewer from 'colorbrewer'; | ||
|
||
var iconv = require('iconv-lite'); |
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.
Can this be a constant?
const MapElement = () => { | ||
|
||
// console.log(colorbrewer.schemeGroups.sequential); | ||
// console.log(colorbrewer) |
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.
Can you remove this line?
|
||
import { TileProviders } from '../lib/TileProviders'; | ||
|
||
import colorbrewer from 'colorbrewer'; |
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.
two spaces after import, awful
|
||
const handleFile = (e) => { | ||
var reader = new FileReader(); | ||
var file = e.target.files[0]; |
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.
Can these be consts?
else { | ||
if ( !apiKey || apiKey !== 'none') { | ||
return ( | ||
<BaseLayer key={index} name={providerName}> |
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.
Is there not anything else we can use for the key?
} | ||
|
||
|
||
// console.log(providerName + ":" + tileUrl) |
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.
should remove old commented out logs
var varUrl = variant.url ? variant.url : tileUrl; | ||
|
||
|
||
// console.log( " -- varFullName: " + varFullName + ": " + varUrl); |
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.
another to be removed
|
||
<Marker position={map !== null ? map.getCenter() : center} draggable={true} animate={true}> | ||
<Popup> | ||
A pretty CSS3 popup. <br /> Easily customizable. |
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.
Is this boiler plate code?
|
||
return ( | ||
|
||
Object.keys(variants).map((varName, varIdx) => { |
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.
Can you change these to variantName
, and variantIndex
? Var
seems too much like Javascript var
keyword.
Add a Map Element that allows you to toggle through different types of tiles.