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: remove legacy report view #1233

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Conversation

IanKrieger
Copy link
Member

Removes legacy reporting view and related code

@IanKrieger IanKrieger requested a review from a team as a code owner June 13, 2024 14:44
Copy link

[puLL-Merge] - brave/ads-ui@1233

Here is the code review for the provided PR diff:

Description

This PR removes several components and routes related to the original campaign reporting view and refactors the code to use the new reporting view across all campaign formats. It also removes some unused analytics components.

Changes

Changes

  • Removes src/components/Collapse/ChangeReportingAlert.tsx which displayed an alert about the updated reporting view
  • Removes src/components/Collapse/CollapseBox.tsx which was a collapsible section used in the original reporting view
  • Updates src/components/Datagrid/renderers.tsx to remove usage of the CampaignExtras type
  • Updates src/routes/campaigns/analytics/CampaignAnalytics.tsx:
    • Removes display of ChangeReportingAlert for non-search campaigns
    • Removes passing of CampaignFormat to determine display
  • Updates routes in src/user/User.tsx:
    • Changes /user/main/campaign/:campaignId route to redirect to /user/main/report/:campaignId
    • Removes duplicate /user/main/report/:campaignId route
  • Removes src/user/adSet/AdSetList.tsx which displayed the list of ad sets in the original reporting view
  • Removes src/user/ads/AdList.tsx which displayed the list of ads in the original reporting view
  • Removes several components under src/user/analytics/analyticsOverview/components/* which were used for the original reporting metrics and charts
  • Removes src/user/analytics/analyticsOverview/lib/* containing library code for original reporting
  • Removes src/user/analytics/analyticsOverview/reports/* containing the original engagement and OS reporting sections
  • Removes src/user/analytics/analyticsOverview/types/index.ts containing types for original reporting
  • Removes src/user/analytics/renderers/index.tsx containing cell renderers for original reporting stats
  • Removes src/user/views/user/AdDetailTable.tsx which was the table component for displaying ad set and ad details
  • Removes src/user/views/user/CampaignDetails.tsx which contained tabs for ad set and ad lists
  • Updates src/user/views/user/CampaignReportViewSelector.tsx:
    • Removes logic for selecting between original and new reporting view
    • Always displays AnalyticsOverview except for NTP-SI format
  • Removes src/user/views/user/reports/OriginalCampaignReportView.tsx which was the top-level component for the original reporting view

Security Hotspots

This PR does not appear to introduce any clear security vulnerabilities. The main changes are removing unused code and consolidating to use the new reporting view.

@IanKrieger IanKrieger added dependencies Pull requests that update a dependency file and removed dependencies Pull requests that update a dependency file labels Jun 13, 2024
Copy link
Collaborator

@tackley tackley left a comment

Choose a reason for hiding this comment

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

Nice!

@IanKrieger IanKrieger merged commit 4059381 into master Jun 17, 2024
8 checks passed
@IanKrieger IanKrieger deleted the feat/remove-legacy-report-view branch June 17, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants