This repository has been archived by the owner on Mar 5, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 89
Additional GA Event and Modal View Tracking #373
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If we decide to make the modals visible at their own URLs so that e.g. the "add your club" modal is visible at "/mozilla-web-clubs/add/", is this solution compatible with that?
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.
Just to clarify, would that mean there are two ways to the display the form? (ie as a modal, and as a standalone page)?
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.
Nope, it'd just be the case that
/mozilla-web-clubs/add
would show you the "darkened" page in the background w/ the modal focused in front of it, and anything that currently opens the modal via JS would instead just be a clickable link to/mozilla-web-clubs/add
. The fancypants router would then trigger the transition to the modal.If that is really weird and nobody else does it then we don't have to... I just like every "thing" on a page to have a URL I guess. It's also a bit annoying to implement in the current architecture so it wouldn't be implemented in v1.
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.
Oh, aside from that, it would theoretically allow us to support people actually using those modals on browsers with JS disabled. Additional work beyond just making the modals have their own URLs would need to be done, though. Again though, not sure whether the modals are considered "progressive" or "required" functionality--I'd think at least the "learn more" would be required, but I suppose we could always have that link be a
mailto:
link w/ the progressive enhancement being the modal...Anyways, that's off-topic for now I guess!
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.
That's interesting, and I've not seen it done before.
If we make that switch, I think we would have pageview tracking via the router, and we could remove the extra tracking in the modal module.
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.
Ok, cool--I just wanted to make sure it wouldn't screw up metrics that we track over time, e.g. because what used to be the '/modal/addclub
"route" in ga suddenly changes to
/mozilla-web-clubs/add`.Thanks!