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

[DRAFT] New Report Design #12

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

zkendall
Copy link
Contributor

@zkendall zkendall commented Feb 29, 2024

Based on discussion here: #10

Note this is based off another existing branch where I added new fields. I can merge them separately or discard that one and continue with this "all-in-one" PR.

@zkendall zkendall changed the title New Report Design [DRAFT] New Report Design Feb 29, 2024
Comment on lines +4 to +15
<style>
.charts-container {
display: flex;
flex-wrap: wrap;
justify-content: space-around;
}

.chart-wrapper {
flex: 0 0 800px; /* Adjust this value based on your desired width, leaving room for margins */
margin-bottom: 10px; /* Space between rows */
}
</style>
Copy link
Contributor Author

@zkendall zkendall Feb 29, 2024

Choose a reason for hiding this comment

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

I added flexible containers so that the charts can wrap in columns and rows based on screen size.

Here is narrow window so one column:

image

Here is large window so many columns

image

<meta charset="utf-8">
</head>
<body style="height: 100%; margin: 0; background-color:#100C2A">
<script type="text/javascript" src="https://fastly.jsdelivr.net/npm/echarts@5.4.3/dist/echarts.min.js"></script>

<p style="color:#fff; font-size: large; text-align:center; margin: 0">Daikin Dashboard</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a little header to make it feel less naked and cramped at the top, since I removed the other date headers.

}
},
],
dataZoom: [
Copy link
Contributor Author

@zkendall zkendall Feb 29, 2024

Choose a reason for hiding this comment

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

Data zoom can be seen working here

image

};

chart.setOption(option);
chart.group = 'ChartGroup1'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Syncing all charts can be seen here

It feels a little noisy. I think with the cross-hair, we don't need the tool tip. I'll see if I can clean that up a bit.
image

Copy link
Contributor Author

@zkendall zkendall Feb 29, 2024

Choose a reason for hiding this comment

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

I removed large tooltip, much better IMO.

But maybe there's a way to style it so it looks more subtle while still giving us information. I think maybe just showing ONLY the Y-axis value without the X axis value and making it less white maybe would help?

image

Copy link
Contributor Author

@zkendall zkendall Mar 1, 2024

Choose a reason for hiding this comment

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

Alternatively: An option is to use the tooltip but make it more subtle. I like this option quite a bit and am going to commit it to the branch :).

Here is the config for the following image.

This removes the horizontal crosshair to reduce noise, reducing padding to reduce size, uses a dark background and light text so it is less glaring. I also added units to it as a bonus.

        tooltip: {
            trigger: 'axis',
            axisPointer: {
                type: 'cross',
                axis: 'x',
            },
            valueFormatter: (value) => value + ' {{.YAxisUnit}}',
            backgroundColor: '#19143a',
            borderColor: '#333',
            padding: 3,
            textStyle: {
                color: '#ccc',
            },
        },
image

Copy link
Contributor Author

@zkendall zkendall Mar 1, 2024

Choose a reason for hiding this comment

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

Another option would be to use a simple string formatter. It's "okay", but I like it less than the above option.

It works by adding this to the tooltip config: formatter: '{b0} -- {c0} {{.YAxisUnit}}',

image

internal/charts/charts.go Outdated Show resolved Hide resolved
)


const deviceId = "FILL ME IN"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aside: I'd like to move deviceId to the config file so I can reference it in tests (like db below) to avoid this placeholder. But I've been putting that off to avoid this PR getting too big.

@@ -16,7 +16,15 @@ type Chart struct {
TemperatureUnit string
}

type ChartV2 struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A number of places I made a V2 of existing components. This is because I didn't want the PR to be destructive. Lemme know if this is a fine strategy.

internal/charts/charts.go Outdated Show resolved Hide resolved
panic(err)
}

periods[i].Period = t.Format("Jan 02 15:04")
Copy link
Contributor Author

@zkendall zkendall Mar 3, 2024

Choose a reason for hiding this comment

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

This Format was a later update in this PR so this format is not seen in other screenshots. But I think this improves readability quite a bit
image

Copy link
Owner

Choose a reason for hiding this comment

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

Looks great!

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