-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
247 interactive chart #255
Conversation
Hi Thiago, thanks again for this. A lot of that refactoring was long overdue. Overall looks good, I haven't run it yet, will tomorrow at hack night. Could you please re-add the
Link: |
Hi @thadk! I've re-added the variant={variant} prop to the TableRow as requested. Let me know if you need any further adjustments. |
@@ -34,27 +32,29 @@ export function AnalysisHeader(props: { usage_data: any }) { | |||
// 3312125.0171753373 | |||
// ]]) | |||
|
|||
const summaryOutputs = props.usage_data?.get('summary_output') | |||
// Extract the summary_output from usage_data | |||
const summaryOutputs = usage_data?.summary_output; |
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.
Future issues:
- Remove
?.
optional chaining operator fromuser_data
- How do we want to handle
?.
optional chaining operators in general? They indicate that something is not as expected, which will have consequences somewhere in the code. Those cases should be handled.
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.
We'll handle this using Zod validation, so we can safely remove the ?.
operator. The schema ensures default values. No need for optional chaining since Zod validates the structure and provides defaults where necessary. The validation is in types/index.ts, ensuring consistency.
This a example:
export const summaryOutputSchema = z.object({
estimated_balance_point: z.number().optional().default('N/A'),
other_fuel_usage: z.number().optional().default('N/A'),
average_indoor_temperature: z.number().optional().default('N/A'),
difference_between_ti_and_tbp: z.number().optional().default('N/A'),
design_temperature: z.number().optional().default('N/A'),
whole_home_heat_loss_rate: z.number().optional().default('N/A'),
standard_deviation_of_heat_loss_rate: z.number().optional().default('N/A'),
average_heat_load: z.number().optional().default('N/A'),
maximum_heat_load: z.number().optional().default('N/A'),
});
Since we have validation with Zod, we no longer need to manually use ?. or write something like this: {summaryOutputs?.average_indoor_temperature ?? 'N/A'} °F
, because Zod will handle it for us.
Let me know what you think!
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.
Briefly talked about tradeoffs of using optional in meeting, esp mentioning required form fields being driven by Zod. To me that means if we take the .optional() approach, we may need multiple zod objects for the same piece.
Let's cover this in #258
const heatingAnalysisTypeRecords = props.usage_data | ||
?.get('billing_records') | ||
?.filter((billingRecord: any) => billingRecord.get('analysis_type') == 1) | ||
const heatingAnalysisTypeRecords = usage_data?.billing_records?.filter( |
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.
const heatingAnalysisTypeRecords = usage_data?.billing_records?.filter( | |
// If heatingAnalysisTypeRecords comes out undefined, it gets handled later | |
const heatingAnalysisTypeRecords = usage_data?.billing_records?.filter( |
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.
Zod solution: #255 (comment)
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.
Partial review
@@ -64,14 +64,14 @@ export function AnalysisHeader(props: { usage_data: any }) { | |||
<div className="item-title-small"> | |||
Average Indoor Temperature <br /> | |||
<div className="item"> | |||
{summaryOutputs?.get('average_indoor_temperature')} °F | |||
</div>{' '} | |||
{summaryOutputs?.average_indoor_temperature} °F |
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.
Handle the case where summaryOutputs is undefined
{summaryOutputs?.average_indoor_temperature} °F | |
{summaryOutputs?.average_indoor_temperature ?? `N/A`} °F |
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.
Zod solution: #255 (comment)
<br /> | ||
Balance Point Temperature | ||
<br /> | ||
<div className="item"> | ||
{summaryOutputs?.get('estimated_balance_point')} °F | ||
</div>{' '} | ||
{summaryOutputs?.estimated_balance_point} °F |
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.
{summaryOutputs?.estimated_balance_point} °F | |
{summaryOutputs?.estimated_balance_point ?? `N/A`} °F |
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.
Zod solution: #255 (comment)
@@ -83,8 +83,8 @@ export function AnalysisHeader(props: { usage_data: any }) { | |||
Daily non-heating Usage <br /> | |||
<div className="item"> | |||
{/* Rounding to two decimal places */} | |||
{summaryOutputs?.get('other_fuel_usage').toFixed(2)} therms | |||
</div>{' '} | |||
{summaryOutputs?.other_fuel_usage?.toFixed(2)} therms |
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.
{summaryOutputs?.other_fuel_usage?.toFixed(2)} therms | |
{summaryOutputs?.other_fuel_usage?.toFixed(2) ?? `N/A`} therms |
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.
Zod solution: #255 (comment)
100 | ||
).toFixed(2)}{' '} | ||
summaryOutputs?.standard_deviation_of_heat_loss_rate * 100 | ||
)?.toFixed(2)}{' '} |
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.
)?.toFixed(2)}{' '} | |
)?.toFixed(2) ?? `N/A`}{' '} |
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.
Zod solution: #255 (comment)
{summaryOutputs?.get('whole_home_heat_loss_rate').toFixed(0)}{' '} | ||
BTU/h-°F | ||
</div>{' '} | ||
{summaryOutputs?.whole_home_heat_loss_rate?.toFixed(0)} BTU/h-°F |
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.
{summaryOutputs?.whole_home_heat_loss_rate?.toFixed(0)} BTU/h-°F | |
{summaryOutputs?.whole_home_heat_loss_rate?.toFixed(0) ?? `N/A`} BTU/h-°F |
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.
Zod solution: #255 (comment)
heat-stack/app/components/ui/heat/CaseSummaryComponents/EnergyUseHistoryChart.tsx
Outdated
Show resolved
Hide resolved
if (period) { | ||
const currentOverride = period.inclusion_override | ||
// Toggle 'inclusion_override' | ||
period.inclusion_override = !currentOverride |
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 are the expected effects from this mutation?
Co-authored-by: TBardini <[email protected]> Co-authored-by: Devin Altobello <[email protected]>
…tter readability and validation. Updated components (AnalysisHeader, EnergyUseHistory, EnergyUseHistoryChart) to handle usage_data as an object instead of Map. Introduced mapToObject function in single.tsx to initiate this change and propagate it to other components. Enhanced Zod type definitions across the components for stronger type validation and clarity. These changes improve the organization and readability of the code related to types in the heat-stack/types directory.
…r with structuredClone() in EnergyUseHistoryChart.tsx
Co-authored-by: Charlie Kelly <[email protected]> Co-authored-by: plocket <[email protected]> Co-authored-by: thomas-davis <[email protected]> Co-authored-by: TBardini <[email protected]>
2343e80
to
6a00207
Compare
EnergyUseHistoryChart
.inclusion_override
by clicking the checkbox.Additionally, refactored
single.tsx
and related components for better code structure:single.tsx
to transformusage_data
from a Map to an object for easier manipulation and validation with Zod.mapToObject
function insingle.tsx
to convert Map to objects, making the data easier to work with and validate. This change was propagated to other components.AnalysisHeader
,EnergyUseHistory
,EnergyUseHistoryChart
) to handleusage_data
as an object instead of a Map.types.ts
file to export Zodz.infer
types, improving type management and code clarity.