1002 data export wrong data types for timestamps#1064
1002 data export wrong data types for timestamps#1064Chitransh31 wants to merge 31 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1002 regarding incorrect data types for timestamps in Excel exports. The changes consolidate redundant sheets into a unified format, add a new "Auswertung" (Analysis) sheet with embedded chart visualization using uPlot, and fix sheet naming conventions.
Changes:
- Converted
exportGraphsToExcelfrom synchronous to async to support chart image generation - Merged separate data and stats sheets into combined sheets with side-by-side layout
- Added new "Auswertung" sheet with embedded chart images and legends generated via uPlot
- Improved sheet naming logic based on graph title, series title, and unit type
| // Wrap to next line if needed | ||
| if (legendX > 1100 && index < allSheetData.length - 1) { | ||
| legendX = 20; | ||
| legendY += 20; | ||
| } |
There was a problem hiding this comment.
The legend wrapping logic on lines 278-282 calculates when to wrap to the next line but doesn't properly check if the canvas height is sufficient for multiple rows of legend items. If there are many series, the legend could overflow the 50px canvas height, causing items to be cut off. Consider dynamically calculating the required height based on the number of items and rows needed.
| export async function exportGraphsToExcel( | ||
| graphDataMap: Map<string, () => GraphExportData | null>, | ||
| groupId: string, | ||
| ): void { | ||
| ): Promise<void> { |
There was a problem hiding this comment.
The function changed from synchronous to async but the handleExport callback in useGraphSync.ts (line 129) doesn't await the result. This means errors from the async function won't be properly caught by the caller. Consider updating the calling code to handle the promise returned by exportGraphsToExcel, or at least add .catch() to handle rejections.
|
|
||
| // Determine column header based on sheet name and unit | ||
| // Extract base name from sheet name (e.g., "Nozzle Temp, Watt" -> "Nozzle") | ||
| let baseName = sheetName; |
There was a problem hiding this comment.
The initial value of baseName is unused, since it is always overwritten.
| let baseName = sheetName; | |
| let baseName: string; |
|
@TheBest6337 pleaase review, and give me an update what you think about styl & ability |
|
@Chitransh31 please fix the build issues that come from code formatting |
There was a problem hiding this comment.
Im not a fan of this implementation. Its too much code that is A not necesarry or an duplicate of something which could have been done only once. B - too much hardcoded stuff... like the Control Version or all the if cases of string for the machines that can easily break because no one expects this stuff in an excel export.
You should use more object based programming, I think that it should be possible to remove most of the hardcoded stuff and get the data from the current graph implementation, which would be way cleaner. Some functions also seem to be pretty long and could be made into their own file, this would make an cleaner implementation and you need less duplicate code. If you need to create small changes to an machine file because of that you can do so, the namespaces are there for a reason or you can get the data from the graph page...
Copilots suggestions are also somewhat true..
| new Date().toLocaleString("de-DE", { | ||
| year: "numeric", | ||
| month: "2-digit", | ||
| day: "2-digit", | ||
| hour: "2-digit", | ||
| minute: "2-digit", | ||
| second: "2-digit", | ||
| }), |
Chitransh31
force-pushed
the
1002-data-export-wrong-data-types-for-timestamps
branch
from
845421f to
4408feb
Compare
|
@Chitransh31 whats your status on this, can I review it? |
Yes, you can review it. |
TheBest6337
left a comment
TheBest6337
left a comment
There was a problem hiding this comment.
Code wise its good, i would put most of the files in an extra folder under graph (excel) for example or excelExport. So you can get an better overview.
When i opened the saved excel file for the Mock-Machine im greeted with an error message:
please fix that
Chitransh31
force-pushed
the
1002-data-export-wrong-data-types-for-timestamps
branch
2 times, most recently
from
c3e1b88 to
8d108e3
Compare
Refactored to move the excel export files in graph/excelExport folder. I'm able to download and open all the graphs. Please verify.
|
…hart image generation using uPlot. Scatter chart with ExcelJs did not work because of limited support and xlsx-chart creates an entirely new excel file for chart generation
…ore and the relevant logs are now passed as a parameter
…f at the analysis sheet
…into their own files for readability and maintainability. added support for pid export as stated in the issue and config export to follow dependency inversion principle and to centralize the required hardcoded values to provide type safe access
…sed the corrupt excel export file. added excel sheet sanitizer to check whether the exported excel file is clean
Chitransh31
force-pushed
the
1002-data-export-wrong-data-types-for-timestamps
branch
from
aacdcd5 to
603513f
Compare
|
@TheBest6337 please review the code. |
|
@Chitransh31 looks good to me, I will still whait with the merge because I want to test it with a larger set of data during production. |
TheBest6337
left a comment
TheBest6337
left a comment
There was a problem hiding this comment.
I just found a bug: When you export once it saves these values but if you then whait a few mins and want to save another set it still only saves to old one. Its possible that you might need to clear some things.
(This was exported at 14:09)
Seems only to happen when staying on the graphs page, if I move to another and then switch back it works as expected.
3 participants
Fixed the sheets' names, merged the redundant sheets and created a new Auswertung sheet with chart image generation using uPlot. Scatter chart with ExcelJs did not work because of limited chart support and xlsx-chart creates an entirely new excel file for chart generation. Fixed colours in the chart and added legends below the chart for the parameters.