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

bugfix run-ad-auction was moved to run-simple-ad-auction.js #535

Conversation

sidneyzanetti
Copy link

Prior to creating a pull request, please follow all the steps in the contributing guide.

Description

Bugfix: run-ad-auction.js was renamed to run-simple-ad-auction.js

Affected services

  • Home
  • News
  • Shop
  • Travel
  • DSP
  • SSP
  • ALL

Other:

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi @sidneyzanetti and reviewers,

I'm currently reviewing this pull request and will provide feedback shortly. In the meantime, here's a summary to help everyone get up to speed:

This pull request addresses a simple renaming of the run-ad-auction.js file to run-simple-ad-auction.js. The intent is purely a renaming operation, to better reflect the functionality of the script. No functional changes are introduced.

Here's a breakdown of the changes:

  • File Renaming: The core change involves renaming run-ad-auction.js to run-simple-ad-auction.js across multiple files and locations.
  • services/ad-tech/src/routes/common/common-router.ts: Line 68 was updated to reflect the new file name in the routing logic. The filePath.endsWith('/run-ad-auction.js') check was changed to filePath.endsWith('/run-simple-ad-auction.js').
  • services/ad-tech/src/views/ssp/run-ad-auction.ejs: Line 41 updated the <script> tag to point to the renamed file: /js/ssp/run-simple-ad-auction.js.
  • services/home/docs/demos/retargeting-remarketing.md: Lines 231 and 234 updated the documentation links and code examples to reflect the new file name.

Summary Table:

File Change
services/ad-tech/src/routes/common/common-router.ts Updated routing logic to use the new file name.
services/ad-tech/src/views/ssp/run-ad-auction.ejs Updated script tag to point to the renamed file.
services/home/docs/demos/retargeting-remarketing.md Updated documentation links and code examples to reflect the new file name.

I'll provide a more detailed review shortly. Thanks!

And now, a little haiku to brighten your day:

Code's gentle flow,
Renamed files, a new name,
Smooth sailing ahead.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request renames run-ad-auction.js to run-simple-ad-auction.js. The changes are correctly implemented across all affected files. However, there are a few minor suggestions for improvement, particularly regarding documentation and consistency. I've also summarized the style guide elements referenced in this review below.

Style Guide Elements Referenced:

Given no specific style guide was provided, I've defaulted to common best practices for each language, including:

  • Typescript: General Typescript best practices, focusing on clear naming and consistent formatting.
  • HTML: Standard HTML best practices for semantic structure and accessibility.
  • Markdown: CommonMark conventions for readability and consistency.

I recommend reviewing the suggested changes, especially the documentation updates, before merging. These improvements will enhance clarity and maintainability.

Copy link
Contributor

@Seburan Seburan left a comment

Choose a reason for hiding this comment

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

lgtm

@sidneyzanetti sidneyzanetti merged commit a762b3d into privacysandbox:dev Jan 24, 2025
2 checks passed
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