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

chore: refactor advertising.js to improve readability #104

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

PoChenKuo
Copy link

@PoChenKuo PoChenKuo commented May 19, 2023

Refactor of advertising.js to make it more readable.

@DanSnow
Please help to review it.

@DanSnow DanSnow self-requested a review May 21, 2023 16:31
src/Advertising.js Show resolved Hide resolved
src/Advertising.js Outdated Show resolved Hide resolved
src/Advertising.js Outdated Show resolved Hide resolved
@PoChenKuo PoChenKuo requested a review from DanSnow May 22, 2023 06:56
@HelloAlexPan HelloAlexPan changed the title chore: break dwon the Advertising.js chore: break down Advertising.js May 22, 2023
@HelloAlexPan
Copy link
Member

Awesome work @PoChenKuo — fix DS issues

image image

@PoChenKuo
Copy link
Author

@DanSnow

To avoid getAdUnits and getDivIdsAndSlots function into static member
I remove the parameters and move them into the function context.
image
8f8a07f

@PoChenKuo
Copy link
Author

@DanSnow
Is anything still need to change?

@HelloAlexPan HelloAlexPan changed the title chore: break down Advertising.js chore: refactor Advertising.js to improve readability May 28, 2023
@HelloAlexPan HelloAlexPan changed the title chore: refactor Advertising.js to improve readability chore: refactor advertising.js to improve readability May 28, 2023
@HelloAlexPan
Copy link
Member

HelloAlexPan commented May 28, 2023

Is anything still need to change?

Thanks for opening this PR @PoChenKuo; this is really great work.

As this is a minor refactor, reviewing this PR is a lower priority, and the team will try to review this and get it merged in the coming weeks.

Feel free to let me know what other improvements you like to be made to this library, either here or as an issue!

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.

3 participants