Skip to content
This repository was archived by the owner on Jan 27, 2025. It is now read-only.

RTA-372 Call bakers when they are present. Use ajax and /j endpoint#25

Merged
melgenek merged 12 commits intomasterfrom
RTA-372
Oct 1, 2020
Merged

RTA-372 Call bakers when they are present. Use ajax and /j endpoint#25
melgenek merged 12 commits intomasterfrom
RTA-372

Conversation

@melgenek
Copy link
Contributor

@melgenek melgenek commented Sep 22, 2020

This pr still uses the embedded ajax and pixel implementations. I'll create a separate pr to make them pluggable.

Related prs:

@melgenek melgenek requested a review from a team September 22, 2020 20:00
@jankoulaga
Copy link
Contributor

@melgenek this looks cool! Are we extracting the ajax to have the same signature as prebid in a separate PR?

@melgenek
Copy link
Contributor Author

@jankoulaga yes. I'll create a separate pr for the extraction. I want to release a dedicated version of lc2 builder with the changes from this pr to keep the changes in each version granular.

*/
function _send (state) {
function _sendAjax (state) {
_sendState(state, 'j', uri => {
Copy link
Contributor

Choose a reason for hiding this comment

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

DOn't we need the onload here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was one line below. I moved the bakers' calls into a separate function to make the code a bit more clear. And now the ajax callback calls the "onload" and "callBakers" https://github.com/LiveIntent/live-connect/pull/25/files#diff-51925b852a9fa34aa4d658fa7cd30c4dR22-R23

Base automatically changed from RTA-286 to master September 23, 2020 13:25
Copy link
Contributor

@gipeshka gipeshka left a comment

Choose a reason for hiding this comment

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

Does this PR contain parts of referer fix?

@melgenek
Copy link
Contributor Author

@gipeshka there was a pr into a branch that is not a master when I merged it the changes were shown for the referrer as well.
I pushed the merge with master, this pr is only about the ajax+/bakers calls now.

@codecov-commenter
Copy link

Codecov Report

Merging #25 into master will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   93.84%   93.97%   +0.13%     
==========================================
  Files          28       29       +1     
  Lines         731      747      +16     
==========================================
+ Hits          686      702      +16     
  Misses         45       45              
Impacted Files Coverage Δ
src/events/error-pixel.js 96.55% <100.00%> (ø)
src/live-connect.js 89.87% <100.00%> (ø)
src/pixel/sender.js 100.00% <100.00%> (ø)
src/utils/pixel.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0303966...a59d98e. Read the comment docs.

@jankoulaga
Copy link
Contributor

@gipeshka can you check this please?

@melgenek melgenek merged commit 1ddf231 into master Oct 1, 2020
@melgenek melgenek deleted the RTA-372 branch October 1, 2020 09:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments