Dylan Blake Assignment 6 #53
Conversation
…ntended. had to change up the package.json in a way that differed from my peers but at least it is working.
blakedylan16
requested review from
erinkelley27,
juliaWoelmer,
evelynzhengg and
cindyd02
| @@ -3,6 +3,9 @@ | |||
| "version": "1.0.0", | |||
| "description": "This repo is used for the design and d3 data visualization training.", | |||
| "main": "index.js", | |||
| "scripts": { | |||
There was a problem hiding this comment.
I had to change it to "start": "node node_modules/http-server/bin/http-server" in order to get it to run using npm start because it wasn't working with what you had here. Not sure why this worked for you and not for me? Dunno
| @@ -3,7 +3,22 @@ | |||
| <title>US Air Quality</title> | |||
| </head> | |||
| <body> | |||
| <br><br><br> | |||
| <div> | |||
| <input type="radio" id="sort-state" name="sort" checked> | |||
There was a problem hiding this comment.
Nice use of labels with the input here and the "for" specifications. Really makes it clear to read and is great from an accessibility standpoint. Not sure you need all of the breaks above. If you want to include some sort of top margin that can be done using styling which is much cleaner
| @@ -1,4 +1,5 @@ | |||
| function createChart(elementId) { | |||
|
|
|||
There was a problem hiding this comment.
Unnecessary newline here
|
|
||
| // read in air quality data | ||
| d3.csv('air_quality.csv').then(function (data) { | ||
| d3.csv('air_quality.csv').then( function(data) { |
There was a problem hiding this comment.
You can get rid of the "function(data)" here and replace it with "(data) =>."
| .attr('y', (_, i) => i * (legendBlockSize+legendSpacing)) | ||
| .attr('fill', d => scaleOrdinal(d)); | ||
|
|
||
| legend.selectAll('text') |
There was a problem hiding this comment.
The legend text is being slightly cut off. For example, it says "Northea" and "Midwes"
| const t = svg.transition() | ||
| .duration(1500); | ||
| const delay = +document.getElementById("delay-input").value; | ||
| if (isNaN(delay)) alert("Can not accept input"); |
There was a problem hiding this comment.
It may be a good idea to give the user an idea of why the input cannot be accepted. Something like "Delay input must be a number"
| <br> | ||
| <div id="class-checkbox-container"> | ||
| <label for="class-1">Class: 1</label> | ||
| <input type="checkbox" id="1" name="class-checkbox" checked><br> |
There was a problem hiding this comment.
A lot of uses of br here that I think could be replaced with styling changes
| .attr('stroke', '#999999') | ||
| .attr('stroke-width', '.5'); | ||
|
|
||
| const stationDots = map.selectAll('circle') |
There was a problem hiding this comment.
Another unnecessary variable
| .style('font-size', legendBlockSize) | ||
| .style('fill', 'black') | ||
| .attr('x', legendBlockSize + 4) | ||
| .attr('y', (_, i) => i * (legendBlockSize+legendSpacing)) |
There was a problem hiding this comment.
Not really a reason for the extra whitespace here?
| .filter(station => | ||
| checkedBoxes.includes(station.class) | ||
| ) | ||
| .filter(station => |
There was a problem hiding this comment.
You should be able to combine these filter statements
Did some interactions and transitions for module 6.