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

YieldOne Bid Adapter: add Flux Wrapper support. #7555

Merged
merged 3 commits into from
Nov 11, 2021

Conversation

kyoya-takei
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

@ChrisHuie
Copy link
Collaborator

@kyoya-takei can you please change back the specific imports for the utils instead of importing the whole file with import * from utils . We went through all bid adapters to add conformity and only import the functions needed for future dead-code elimintation. Thanks

@kyoya-takei
Copy link
Contributor Author

kyoya-takei commented Oct 13, 2021

@ChrisHuie

@kyoya-takei can you please change back the specific imports for the utils instead of importing the whole file with import * from utils . We went through all bid adapters to add conformity and only import the functions needed for future dead-code elimintation. Thanks

This solved.
Please check.
Thanks

@haruka-yamashita2
Copy link
Contributor

Hi, team (@umakajan )

How long will the review take?
It's been a long time since we made the request, so please could you tell us the date when you will finish to review.

Thank you.
Regards, Haruka

@ChrisHuie ChrisHuie removed the request for review from umakajan October 28, 2021 14:07
@ChrisHuie ChrisHuie requested a review from ncolletti October 28, 2021 14:07
@ChrisHuie ChrisHuie requested a review from umakajan October 28, 2021 14:08
@ChrisHuie
Copy link
Collaborator

@haruka-yamashita2 I added another possible reviewer to help get this reviewed.

@haruka-yamashita2
Copy link
Contributor

Hi, team (@ChrisHuie )

Excuse me for being busy, but is it likely that reviews will still take place?
How long will the review take?

Thank you.
Regards, Haruka

Copy link
Contributor

@umakajan umakajan left a comment

Choose a reason for hiding this comment

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

Minor comments, but this PR looks good

}

const result = playerSize || DEFAULT_VIDEO_SIZE;
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can simply return playerSize || DEFAULT_VIDEO_SIZE here

/**
* @param {Object} bidRequest -
* @param {boolean} [enabledOldFormat = true] - default: `true`.
* @return {{w: number, h: number}} -
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing enabledFlux here

}

const result = parseSizesInput(sizes).join(',');
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Following the other comments, might be cleaner to simply return parseSizesInput(sizes).join(',');

Copy link
Contributor

@ncolletti ncolletti left a comment

Choose a reason for hiding this comment

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

lgtm

@ncolletti
Copy link
Contributor

sorry for the delay, this looks good. @haruka-yamashita2 will wait to merge until you confirm you'd like to attend to the minor feedback from @umakajan

@haruka-yamashita2
Copy link
Contributor

@umakajan @ncolletti
Thank you for your feedback.
I confirmed that, this look good, too.
Who should modify code? we should modify code and request for merge again?

Thank you.
Regards, Haruka

@umakajan
Copy link
Contributor

umakajan commented Nov 9, 2021

@haruka-yamashita2 these are minor code changes, so you're welcome to change them later by letting us know or simply make the changes and add a commit to this PR. We can merge the PR after.

@haruka-yamashita2
Copy link
Contributor

@umakajan
OK, I pushed commit. Please, confirm.

Thank you.
Regards, Haruka

@ncolletti ncolletti merged commit 38fdca9 into prebid:master Nov 11, 2021
@haruka-yamashita2 haruka-yamashita2 deleted the feature/yieldoneBidAdapter branch November 12, 2021 03:01
cpabst pushed a commit to sovrn/Prebid.js that referenced this pull request Jan 10, 2022
* YieldOne Bid Adapter: add Flux Wrapper support.

* YieldOne Bid Adapter: fix "import utils functions as needed and not the whole module (prebid#7502)"

* YieldOne Bid Adapter: fix minor feedback.

Co-authored-by: kenichi-ichijo <kenichi-ichijo@dac.co.jp>
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.

6 participants