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

Personalization DOM adjuments #31

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Personalization DOM adjuments #31

wants to merge 8 commits into from

Conversation

doublejosh
Copy link
Contributor

@doublejosh doublejosh commented May 8, 2017

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...

groucho.config.adjust = [{
  dataAttribute: 'groucho-region',
  userProperty: 'user.region',
  paramOverride: 'utm_region',
  helperCallback: myNameSpace.regionCodeToName,
}];
<div class="groucho-personal">
  <span data-groucho-region="north-america">Self-Important Tag Line</span>
  <span data-groucho-region="europe">Multicultural Tag Line</span>
  <span data-groucho-region="APAC">Appropriate Tag Line</span>
  <span data-groucho-region="default">Standard Tag Line</span>
</div>

OR

<span class="groucho-personal" data-groucho-region="north-america">Self-Important Tag Line</span>

...the class ensure explicit intent to hide content and allows marking topical content for recording interest but NOT adjusting visually.

@doublejosh
Copy link
Contributor Author

Down to 82% code coverage 😢

Copy link
Contributor

@Etroid Etroid left a 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;
Copy link
Contributor

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)

Copy link
Contributor Author

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') {
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • nit, not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jshint

Copy link
Contributor

Choose a reason for hiding this comment

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

@doublejosh
Copy link
Contributor Author

doublejosh commented Jun 27, 2017

@Etroid I'll need to make some changes after the thread about DOM choices...
#32

Looking to partner a bit with folks to make very sustainable choices.

@doublejosh
Copy link
Contributor Author

  • Would rather separate the param overriding of user preferences out of the adjustment configs, so they can be used for general user property sets (if empty). That config would need to be independent and shared.

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.

None yet

3 participants