-
Notifications
You must be signed in to change notification settings - Fork 134
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
feat: Added Fbclid config option #338
Conversation
@jarrenpatao How did you test your PR btw? |
@jarrenpatao it looks like this is failing at the lint checks (@kelsonpw it's working super well!) - could you run |
@haoliu-amp I did |
if (utils.isEmptyString(fbclid)) { | ||
return; | ||
} | ||
var fbclidProperties = { fbclid: fbclid }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our practice is to add the quote? @kelvin-lu @jooohhn ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ this got updated with @kelsonpw's linting - if the key is a single word (e.g. not { 'fb click id': fbclid }
), this will update to without quotes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! This looks great and we'll talk about deploying this when we're back (it's planned for part of the deep dive to look at the improved build system @jooohhn created)
Summary
includeFbclid
option to configuration options #225Checklist