Skip to content
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

Replacing Fossil Gas in all text, might be good? #131

Closed
wants to merge 1 commit into from

Conversation

sburnschicago
Copy link
Collaborator

@sburnschicago sburnschicago commented Oct 16, 2024

First PR, yay?

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code (Not much coding)
  • I have commented my code, particularly in hard-to-understand areas (No commenting, just changing Natural to Fossil)
  • [] I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes To my limited ability. Built the site, seemed fine. Ran run_all.sh, didn't seem to create too many errors, and did output this: in this file, I believe building-statistics-by-property-type.json
    "FossilGasUse":{
    "count":2549.0,
    "mean":11076092.1999999993,
    "std":21654400.5,
    "min":0.0,
    "max":412348801.5,
    "twentyFifthPercentile":2913684.2999999998,
    "median":5818399.5999999996,
    "seventyFifthPercentile":11964909.1999999993

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Testing Instructions

Please describe the tests/QA that you did to verify your changes. Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

First PR, yay?

- [X] My code follows the style guidelines of this project
- [X] I have performed a self-review of my own code (Not much coding)
- [ ] I have commented my code, particularly in hard-to-understand areas (No commenting, just changing Natural to Fossil)
- [] I have added tests that prove my fix is effective or that my feature works
- [X] New and existing unit tests pass locally with my changes
To my limited ability. Built the site, seemed fine. Ran run_all.sh, didn't seem to create too many errors, and did output this: in this file, I believe building-statistics-by-property-type.json
    "FossilGasUse":{
        "count":2549.0,
        "mean":11076092.1999999993,
        "std":21654400.5,
        "min":0.0,
        "max":412348801.5,
        "twentyFifthPercentile":2913684.2999999998,
        "median":5818399.5999999996,
        "seventyFifthPercentile":11964909.1999999993
@sburnschicago sburnschicago requested a review from vkoves October 16, 2024 02:42
@sburnschicago sburnschicago self-assigned this Oct 16, 2024
Copy link

netlify bot commented Oct 16, 2024

Deploy Preview for radiant-cucurucho-d09bae ready!

Name Link
🔨 Latest commit ef1ee0c
🔍 Latest deploy log https://app.netlify.com/sites/radiant-cucurucho-d09bae/deploys/670f28219e657d00084202ae
😎 Deploy Preview https://deploy-preview-131--radiant-cucurucho-d09bae.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -23,7 +23,7 @@ unit information - here's a table of the mapping so you can see the units clearl
: Water Use (kGal) : WaterUse :
: ENERGY STAR Score : ENERGYSTARScore :
: Electricity Use (kBtu) : ElectricityUse :
: Natural Gas Use (kBtu) : NaturalGasUse :
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually would not recommend changing this - we don't need to rename the data in our data pipeline, because that'll make it harder for folks to compare parsed data to the raw city data. Could you revert any changes that tweak the data?

@sburnschicago
Copy link
Collaborator Author

sburnschicago commented Oct 19, 2024 via email

@vkoves
Copy link
Owner

vkoves commented Nov 18, 2024

@sburnschicago - I wanted to check in on this, would you still be able to fix up this PR?

@sburnschicago
Copy link
Collaborator Author

Hey Victor, sorry for the delay. Got sidetracked learning other things (mainly AI). I think I will need assistance with a couple of things. These static images, I don't have the best image editing software: static/il-power-chart.webp and /static/il-power-chart.png are static files, and need better editing. That's not the main issue though.

TLDR: The data labels feed to the pie charts, more complicated than I thought. Beyond simple editing. Question for you is: Level of priority to get this done? Let me continue to mess with it? Or just get it done?

The issue is that natural gas label feeds directly into the pie chart labels. I think I know what needs to be done, but not how to do it. I was asking an AI editor (Cline) to assist, and looking at piechart.vue, and this was their conclusion:

The user provided a Vue component that uses D3.js to generate a pie chart. The "Natural Gas" label is dynamically added within the renderGraph function. The code is complete and correctly identifies the location where the label is generated. The task is complete.

The "Natural Gas" label in the dynamically generated pie chart SVG is created within the renderGraph function of the provided Vue component. The label is dynamically added to the SVG based on the data passed to the component's graphData prop. The specific text "Natural Gas" is part of the data structure passed to this function.

Me: Ah, where is this passed in, what line?

The user wants to know the specific line where the "Natural Gas" label data is passed into the component. This requires examining how the graphData prop is used. I will re-examine the provided code.

Task Completed
The "Natural Gas" label data is not directly passed as a string. Instead, it's part of the graphData prop, which is an array of IPieSlice objects. Each IPieSlice object has a label property. The exact line where this data is passed depends on where the PieChart component is used. The graphData prop is defined on line 20 of the provided Vue component (@prop({required: true}) graphData!: Array;), and the data is used within the renderGraph function (lines 60-110). To find the exact line where the data is passed, you need to examine the component's usage in other parts of the application.

Me: Explain this to me more, My concern is that, they want to retain the label of Natural Gas, in the data source, but have the front end of this be called Fossil Gas, how significant of a rewrite of code is this?

AI: The user wants to understand the effort required to change the frontend label to "Fossil Gas" while keeping "Natural Gas" in the data source. This requires modifying the renderGraph function to conditionally change the label based on the data source. It's not a significant rewrite, but it does require careful modification of the existing code.

AI: To change the frontend label to "Fossil Gas" while retaining "Natural Gas" in the data source, you would need to modify the renderGraph function within the PieChart component. Specifically, you would modify the html function within the this.svg.selectAll('mySlices').data(dataReady).enter().append('text') section (lines 78-100). Instead of directly using data.label, you would introduce a conditional statement to check if the label is "Natural Gas" and replace it with "Fossil Gas" for display purposes only. This change would not require a significant rewrite, but careful consideration is needed to ensure the logic remains correct and doesn't introduce unintended side effects. The data source would remain unchanged.

I took a crack at editing this tonight, not successful.

@vkoves
Copy link
Owner

vkoves commented Nov 18, 2024

@sburnschicago - you can just skip anything that's too challenging to do - I think getting 80% of this done and noting any spots that you couldn't fix for me to explore later is better than waiting to get more of it in!

@sburnschicago
Copy link
Collaborator Author

Understood, thanks for the guidance! I'll take some more cracks at it, see if anything passable comes out. Good learning experience. I'm trying to be sensitive to the overall structure, and what I know and don't know how to do.

@sburnschicago
Copy link
Collaborator Author

Hey @vkoves No progress, sorry. Covering for another colleague at work while she has been traveling abroad. I'm going to set a deadline of next Tuesday on this. Nothing like a deadline.

@vkoves
Copy link
Owner

vkoves commented Dec 4, 2024

@sburnschicago - sounds good, I'll hold you to it!

@sburnschicago
Copy link
Collaborator Author

See try 2: #141

@sburnschicago
Copy link
Collaborator Author

closing this pull request, duplicative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants