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

feat(dh): adjustments to settlement report #3846

Merged
merged 14 commits into from
Jan 7, 2025
Merged

Conversation

dzhavat
Copy link
Contributor

@dzhavat dzhavat commented Dec 18, 2024

Description

DataHub

  • move period selection to the top
  • fetch grid areas based on the selected period

References

  • #0000

Copy link

nx-cloud bot commented Dec 18, 2024

View your CI Pipeline Execution ↗ for commit 3bc2f08.

Command Status Duration Result
nx build app-dh ✅ Succeeded 1m 16s View ↗

☁️ Nx Cloud last updated this comment at 2025-01-07 07:29:55 UTC

@dzhavat dzhavat marked this pull request as ready for review December 19, 2024 07:57
mimse
mimse previously approved these changes Dec 19, 2024
Copy link
Contributor

@mimse mimse left a comment

Choose a reason for hiding this comment

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

Suggestion: I would not add the grid area query to shared. I think it was a mistake to move the queries away from where they are used. Better to have more specific query closer to the there are used, IMO.

@dzhavat
Copy link
Contributor Author

dzhavat commented Jan 7, 2025

Placing the query under Settlement reports now makes sense but it's also going to be used in Calculations very soon. In this case "Settlement reports" is not the best place anymore. Maybe "wholesale/data-access-graphql" makes more sense.

@mimse
Copy link
Contributor

mimse commented Jan 7, 2025

I would have two query with the same content, but with different names. ex. GetGridAreasForSettlementReports and GetGridAreasForCalculations. Then dont the road if new fields a are added to only one of them, it do not over fetch data in the other because they are separated

@dzhavat
Copy link
Contributor Author

dzhavat commented Jan 7, 2025

Interesting. Will need to think about it because the query right now is only used in a util function that maps the result to dropdown options. The function can of course be modified to take the query document as an input.

@dzhavat dzhavat merged commit af90c42 into main Jan 7, 2025
30 checks passed
@dzhavat dzhavat deleted the feat-settlement-reports-v55 branch January 7, 2025 08:36
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