-
Notifications
You must be signed in to change notification settings - Fork 1
Merge long going React Migration Branch to master #131
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
Conversation
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Christian Oertel <christian.oertel@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
does not work correclty yet Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Use "npm run format" to run it Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: l-1squared <30831153+l-1squared@users.noreply.github.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Test is green wrongly currently :( Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
- moved all Accordion controls to the Scenario component - unified syntax for component properties - Put sub-components into dedicated files - Removed (currently) unnecessary properties to avoid confusion Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
- introduced test data generation methods - fixed unnecessary casting Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
- also implemented custom queries for icons - also moved a test from scenario testing that is better suited for ScenarioHead Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Since we plan to revisit all tests these tests should be fixed or replaced at some point Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
react-scripts causes audit warnings that are not relevant since it is a build tool. See here: facebook/create-react-app#11174 Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
- statistics are now calculated at ScenarioCollectionHead level - extracted StatisticsBreadcrumbs into own component - adapted properties to be consistent with the rest of the document Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
- removed ACTION_ITEMS to be organized in Jira - removed obsolete README_WR24.md - moved createSampleScenario to testdata file since it is more fitting - removed unclear TODO Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
e0d3ddb
to
1471cf2
Compare
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.
Pull Request Overview
This PR merges the long-running React migration branch into master, rebases and cleans up unsigned commits, and aligns components with React Router v6 and updated utilities.
- Introduces React Router (
router.tsx
) and updates the app entry point to useRouterProvider
. - Refactors runtime formatting utilities and removes legacy view components.
- Establishes a
repository
abstraction for JSON-backed report and tag data.
Reviewed Changes
Copilot reviewed 39 out of 44 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
new/src/router.tsx | Adds React Router createBrowserRouter . |
new/src/index.tsx | Replaces direct <App /> with <RouterProvider/> . |
new/src/repository.ts | Implements repository for JSON fixtures. |
new/src/components/utils.ts | Extracts and generalizes runtime formatting. |
new/src/components/casesTable/CasesTable.tsx | Adds placeholder for the new CasesTable component. |
Files not reviewed (1)
- .idea/misc.xml: Language not supported
Comments suppressed due to low confidence (2)
new/src/components/casesTable/CasesTable.tsx:28
- Component functions should be PascalCase. Rename
casesTable
toCasesTable
to match React conventions.
export function casesTable(props: CasesTableProps) {
new/src/components/Scenarios/ScenarioHead.tsx:27
- The new millisecond display via
addRuntimeInMilliseconds
isn’t covered by tests. Consider adding unit tests for sub-second durations to ensure this behavior remains correct.
? addRuntimeInMilliseconds(scenario.scenarioCases[0].durationInNanos)
treshhold: number, | ||
divisor: number, | ||
unitOfMeasure: string | ||
): string { | ||
return durationInNanos > treshhold |
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.
The variable name treshhold
is misspelled. Consider renaming it to threshold
to match common terminology.
treshhold: number, | |
divisor: number, | |
unitOfMeasure: string | |
): string { | |
return durationInNanos > treshhold | |
threshold: number, | |
divisor: number, | |
unitOfMeasure: string | |
): string { | |
return durationInNanos > threshold |
Copilot uses AI. Check for mistakes.
|
||
export const repository = { | ||
getReport(): JsonReport { | ||
return jGivenReport as unknown as JsonReport; |
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.
getReport
casts only jGivenReport
to JsonReport
but the tagFile
field on JsonReport
is never populated. Consider returning both scenarios
and tagFile
together or update the interface.
return jGivenReport as unknown as JsonReport; | |
return { | |
scenarios: jGivenReport.scenarios.map(r => r as ReportModel), | |
tagFile: tagFile | |
}; |
Copilot uses AI. Check for mistakes.
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.
Uhh boy,
that is so much code to look through.
I don't think I can make an accurate assesment of the code (lack of time) and would suggest fixing forward after merch.
Hower for that I would like to not have skipped tests. Currently we are skipping twe entire test suits. Can we change that?
.idea/misc.xml
Outdated
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.
Why is this file here?
Signed-off-by: Robert Schüler <robert.schueler@tngtech.com>
We should not have a master branch and a React Migration branch developed next to each other. So we need to merge the React Migration Branch.
I did the following: