-
Notifications
You must be signed in to change notification settings - Fork 7
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
Personalization DOM adjuments #31
base: master
Are you sure you want to change the base?
Conversation
Down to 82% code coverage 😢 |
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.
First pass look 🆗
dist/groucho.js
Outdated
if (typeof dataLayer !== 'undefined') { | ||
for (var i in trackIds) { | ||
// Safety measure. | ||
if (!trackIds.hasOwnProperty(i)) continue; |
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 seems overkill if you're using
for (var x in list)
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.
jshint says otherwise ;)
dist/groucho.js
Outdated
groucho.trackClicks = function () { | ||
if (!groucho.config.lastClicked) return; | ||
// Bind click event to configured selector. | ||
if (typeof $.fn.click === 'function') { |
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.
- Is this just checking whether or not jQuery is enabled. Seems redundant imho.
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 library works across many versions of jQuery and other selector libraries as available dependencies, so this kind of thing ends required.
dist/groucho.js
Outdated
* @param {object} data | ||
* @param {boolean} keepExisting | ||
* Default is to overwrite value. | ||
* @param {number} ttl |
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.
- docblock
src/favorites.js
Outdated
*/ | ||
function isEmpty(obj) { | ||
for (var prop in obj) { | ||
if (obj.hasOwnProperty(prop)) { |
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.
- nit, not necessary?
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.
jshint
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 change allows easily personalized pages via simple HTML attributes based on a user's preferences (or overridden via URL params for transactional/marketing links and debugging).
The DOM choices were discussed here: #32
Docs updates are staged here: Full-Docs, they will no longer be included in README files within the codebase.
Here's the crux...
OR
...the class ensure explicit intent to hide content and allows marking topical content for recording interest but NOT adjusting visually.