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: Add Legacy Report View #1482

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

dhaselhan
Copy link
Collaborator

  • Add new brancher to control legacy vs non-legacy view
  • Split HistoryCard to its own component for re-usability and add tests
  • Update legacy view with legacy assessment card
  • Sent legacy_id to the front end

@dhaselhan dhaselhan requested a review from AlexZorkin December 17, 2024 22:24
Copy link

github-actions bot commented Dec 17, 2024

Frontend Test Results

  1 files  ± 0  119 suites  +3   42s ⏱️ +3s
411 tests +21  391 ✅ +21  20 💤 ±0  0 ❌ ±0 
413 runs  +21  393 ✅ +21  20 💤 ±0  0 ❌ ±0 

Results for commit 60b16dd. ± Comparison against base commit 2dca338.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 17, 2024

Backend Test Results

502 tests  ±0   502 ✅ ±0   1m 58s ⏱️ +7s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 60b16dd. ± Comparison against base commit 2dca338.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@AlexZorkin AlexZorkin left a comment

Choose a reason for hiding this comment

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

Looks great, only 2 comments.

@@ -1,5 +1,6 @@
export const config = {
api_base: 'http://localhost:8000/api',
tfrs_base: 'http://localhost:3001',
Copy link
Collaborator

Choose a reason for hiding this comment

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

in local we're using port 3000

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I set this to 3001 as LCFS also runs on 3000 so if you were to run both locally at the same time TFRS would need to be on something else. Should I set them to the same?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do like the idea but there are quite a few places we would have to update this. vite config, cypress config, application settings, and a few more spots. Maybe leave it as 3000 and then adjust locally when testing this hybrid code. If you want to go through and update it everywhere im not opposed but we would have to test thoroughly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to 3000.

frontend/src/App.jsx Outdated Show resolved Hide resolved
@dhaselhan dhaselhan force-pushed the feat/daniel-legacy-report-view-1396 branch 2 times, most recently from 1e52ca9 to d07e12d Compare December 18, 2024 18:42
@dhaselhan dhaselhan requested a review from AlexZorkin December 18, 2024 18:43
* Add new brancher to control legacy vs non-legacy view
* Split HistoryCard to its own component for re-usability and add tests
* Update legacy view with legacy assessment card
* Sent legacy_id to the front end
* Rename ComplianceReportViewSelector
@dhaselhan dhaselhan force-pushed the feat/daniel-legacy-report-view-1396 branch from 40c4ab2 to d10b9fe Compare December 19, 2024 18:55
Copy link
Collaborator

@AlexZorkin AlexZorkin left a comment

Choose a reason for hiding this comment

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

👍

@dhaselhan dhaselhan merged commit a98abf4 into release-0.2.0 Dec 19, 2024
11 checks passed
@dhaselhan dhaselhan deleted the feat/daniel-legacy-report-view-1396 branch December 19, 2024 22:57
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