-
Notifications
You must be signed in to change notification settings - Fork 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
planned solar projects #45
Conversation
✅ Deploy Preview for il-solar-map ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@derekeder - not related to this, but I wanted to mention the active state of each toggle definitely looks like it would fail a contrast check. I also wonder if it's worth putting an explainer somewhere of what/how you get "planned" projects. Like what's the certainty those would happen and how far out might they be? |
@vkoves good catch! i'll open a separate PR for the contrasting buttons and pick it up in another PR. would you believe its a bootstrap default!? Agreed on the language too. I will ping the clean jobs folks and see what the say. That makes sense to address in this PR |
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 have some starting comments, but I think I'll need a good bit more documentation to understand what your doing where, since I'm just jumping into this for the first time. From testing it seems fine though!
cur_project["kw"] = clean_number(row["Project Size AC kW"]) | ||
cur_project["census_tract"] = row["Census Tract Code"] | ||
if project_type == "planned": | ||
cur_project["energization_date"] = row["Scheduled Energization Date"] |
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 try to avoid magic strings, so I'd consider making a constant for the available columns in your data, as otherwise someone could slip in a typo on one specific check
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 say more here? this script is combining 3 different CSVs and aligning the appropriate columns into a shared form
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.
What I mean is that hard coding energization_date
and Scheduled Energ...
is dangerous, since someone could be referencing those elsewhere, make a typo, and break something. Constants, like OrigCols.EnegDate
prevent that, since the variable would bomb out if you have a typo.
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, i understand. that's easier to do in javascript due to the way they interpret json data. it would take some code to turn each of these string values in the python dict to act as variables, so probably not worth it.
<br /><br /> | ||
<h5>Select status</h5> | ||
<div id="status-select" class="btn-group flex-wrap" role="group" aria-label="Status"> | ||
<button value="energized" type="button" class="btn btn-light active">Energized</button> |
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 an issue with your other buttons as well, but worth noting a toggle button like this should use aria-pressed
to indicate to screen readers when it's active or not. Accessibility might just be a separate task though
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 will open this up as a separate issue to address
@@ -12,6 +12,8 @@ <h1>Reference Tables</h1> | |||
<li><a href="#il-senate">IL State Senate Districts</a></li> | |||
</ul> | |||
|
|||
<p>Note, all values are in kilowatts (kW). Columns with a <code>PL_</code> prefix are planned and not yet energized projects.</p> |
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.
Minor, but wanted to mention you can add thead { position: sticky; top: 0; }
to this table to have the headers and filters stick, which makes it a lot easier to read and understand
The column titles are also a little technical, I don't know what CS
means, and I think using titlecase and human words helps:
Lastly please add units! Otherwise it's unclear what each thing is referencing, and I thought it was the count of projects, not kW
of capacity
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.
adding kW
to each value is a bit more involved if we want the sorting to work, so i'll open a separate issue
js/map.js
Outdated
</tr> | ||
</thead> | ||
<tbody> | ||
<tr> | ||
<td>Total</td> | ||
<td><span class='float-end'>${props.total_kw.toLocaleString()}kW</span></td> | ||
<td><span class='float-end'>${props.total_count.toLocaleString()}</span></td> | ||
<td><span class='float-end'>${props.planned_total_kw.toLocaleString()}kW</span></td> |
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.
A space before kw
would make these all a lot readable in my opinion, e.g. 49,038kW vs 49,038 kW.
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.
good call
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.
So it looks like you only did this on the total, I meant it for the whole table 😆
I'll leave that for later though
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 woops - i'll get that fixed
</tr> | ||
</tbody> | ||
</table> | ||
` | ||
} | ||
|
||
function updateLegend(layerSource, category){ | ||
let legendText = `<strong>${friendly_category_names[category]} kW of solar installed<br />by ${friendly_geography_names[layerSource]}</strong>` | ||
function updateLegend(layerSource, category, status){ |
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.
Oh no, jQuery 😭
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 using it for a few plugins I need:
- jquery csv for converting a csv into a list of javascript objects
- jquery address for updating and loading in query string parameters
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.
One day, we can switch it to React or Vue 😆
@vkoves thx for the review. I addressed or commented (and opened PRs) for your comments. we can discuss tonight at hack night |
@derekeder - I also noticed that you changed the scale, which means that there's more faint colors overall, is this okay?
|
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.
Looks good to me! I'm not sure how best to validate the data other than to compare the data to prod, and it looks the same but fainter (see my comment about shifting the scale). I'd say anything else (adding space before kw
on each row of the table, the map color scale) can be done in another PR!
@vkoves thx! addressed the simple feedback. also made the buckets a little closer to what they were before. you're right - the map did look pretty light |
Tasks to implement:
dg_small
,dg_large
,cs
andutility
Testing instructions