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

Add techmix per sector plot #38

Merged
merged 21 commits into from
Sep 24, 2024
Merged

Add techmix per sector plot #38

merged 21 commits into from
Sep 24, 2024

Conversation

MonikaFu
Copy link
Contributor

@MonikaFu MonikaFu commented Sep 3, 2024

Add techmix_sector function to the dashboard together with the plot data.

Note that the case of empty data selection is not handled elegantly at this moment. To be handled at a later stage. See #40.

@MonikaFu MonikaFu marked this pull request as ready for review September 23, 2024 10:20
<option value="WEO2023">WEO2023</option>
<option value="GECO2023">GECO2023</option>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changing order to scenario source for which techmix appears.

@jdhoffa
Copy link
Member

jdhoffa commented Sep 23, 2024

Copy link
Member

@jdhoffa jdhoffa left a comment

Choose a reason for hiding this comment

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

A few NIT comments, but generally looks decent :-)

I think only the TODO would be blocking for now.

export class techmix_sector {
constructor(container, data) {
// Data needs to be ordered on year (increasing)
// Data needs to have two years in
Copy link
Member

Choose a reason for hiding this comment

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

Comment seems to be missing crucial info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? These are comments I put in about data requirements for this function. They are not exhaustive though. During documenting/refactoring stage we should probably put in tests to check if all the conditions for data are met. At the same time this function is not meant to be used by general users like functions in R packages so I am not sure to what level of detail we should document it.

Copy link
Member

@jdhoffa jdhoffa Sep 23, 2024

Choose a reason for hiding this comment

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

All I meant is that "Data needs to have two years in" seems like it isn't a complete sentence. Two years in ... what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

forgive my English :D I'll make it clearer

item
);
});
// TODO: make sure that this captures all possible technologies
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to handle this TODO?

@MonikaFu MonikaFu requested a review from jdhoffa September 24, 2024 11:54
.range(d3.schemeSpectral[subgroups.length])
.unknown('#ccc');

// Add rectangles for each stacked bar - TODO: rewrite into two
Copy link
Member

Choose a reason for hiding this comment

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

NIT: @MonikaFu in general, we should aim to NOT have any TODOs on main. So my suggestion would be to please review all remaining TODOs in the comments and either:

  • handle them (if directly relevant to this particular PR)
    OR
  • open a new GH issue explaining what should be done

@MonikaFu MonikaFu requested a review from jdhoffa September 24, 2024 13:28
@MonikaFu MonikaFu merged commit be9b395 into main Sep 24, 2024
4 of 5 checks passed
@MonikaFu MonikaFu deleted the add-techmix-sector branch September 24, 2024 13:50
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