Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

First commit #1

Merged
merged 5 commits into from
Feb 13, 2018
Merged

First commit #1

merged 5 commits into from
Feb 13, 2018

Conversation

jaiminpanchal27
Copy link
Collaborator

First commit.

  • Includes gulp setup
  • Creative.js

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

Needs some updates. Also please add tests. Recommend we use jest maybe for an easy lightweight test framework. Might need to separate logic from the DOM functions.

@@ -0,0 +1,8 @@
{
"presets": ["env"],
Copy link
Member

Choose a reason for hiding this comment

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

from the docs: babel-preset-env behaves exactly the same as babel-preset-latest (or babel-preset-es2015, babel-preset-es2016, and babel-preset-es2017 together).
Is this what we want?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They have deprecated babel-preset-es2015 and advised to use babel-preset-env

Copy link
Member

Choose a reason for hiding this comment

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

ok but what's that do to the file size?

});

gulp.task('build-prod', ['clean'], () => {
var cloned = _.cloneDeep(webpackConfig);
Copy link
Member

Choose a reason for hiding this comment

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

why do you need to clone this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For prod we remove source-maps from webpack config. So we clone the object and do not modify original webpack config.

package.json Outdated
"name": "prebid-universal-creative",
"version": "0.1.0",
"description": "",
"main": "index.js",
Copy link
Member

Choose a reason for hiding this comment

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

this should be dist/creative.js I think

src/creative.js Outdated
/**
* @param {object} doc
* @param {string} adId
* @param {object} dataObject
Copy link
Member

Choose a reason for hiding this comment

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

let's break out the jsDoc for dataObject

src/creative.js Outdated
} else if (environment.isCrossDomain()) {
renderCrossDomain(adId, dataObject.pubUrl);
} else {
// assume legacy?
Copy link
Member

Choose a reason for hiding this comment

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

remove comment

src/creative.js Outdated
utils.sendRequest(adUrl, handler);
}

// function render() {
Copy link
Member

Choose a reason for hiding this comment

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

Drop dead code

/**
* @returns true if we are running on the top window at dispatch time
*/
function isCodeOnPage() {
Copy link
Member

Choose a reason for hiding this comment

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

rename to something like isTopWindow

* Return true if we are in an iframe and can't access the top window.
*/
export function isCrossDomain() {
return window.top !== window && !window.frameElement;
Copy link
Member

Choose a reason for hiding this comment

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

does this throw an exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No it won't throw exception.

/**
* Return true if we cannot document.write to a child iframe (this implies no allow-same-origin)
*/
function isSuperSandboxedIframe() {
Copy link
Member

Choose a reason for hiding this comment

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

unused function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

src/utils.js Outdated

var escapedUrl = encodeURI(url);
var img = '<div style="position:absolute;left:0px;top:0px;visibility:hidden;">';
img += '<img src="' + escapedUrl + '"></div>';
Copy link
Member

Choose a reason for hiding this comment

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

could use string templates here

@jaiminpanchal27
Copy link
Collaborator Author

@mkendall07 Updated the code. We can merge this one and create separate PR for adding test framework.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants