Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
mmv08
left a comment
mmv08
left a comment
There was a problem hiding this comment.
All good, just a few minor things
Could you please also fix eslint issues from the bot? They're minor and shouldn't take a lot of time
| } | ||
| } | ||
| type Props = { | ||
| addressBook: unknown |
There was a problem hiding this comment.
We have a type for addressBook
There was a problem hiding this comment.
I'm making use of it when it calls to getOwnersWithNameFromAddressBook, but If I define it at the Props level it fails. A refactor it's needed, but we should not take care of that in this PR.
|
|
||
| const toggleSidebar = () => { | ||
| if (!isOpen) { | ||
| trackPageEvent({ action: SAFE_NAVIGATION_EVENT, category: 'SafeListSidebar' }) |
There was a problem hiding this comment.
I think it would be better if we write the category normally instead of following PascalCase
There was a problem hiding this comment.
I added it like so because it should be easier to filter in GA dashboard.
There was a problem hiding this comment.
The last time I used analytics there were no difficulties filtering events written nicely with spaces.
src/utils/googleAnalytics.ts
Outdated
| ) | ||
|
|
||
| return { trackPage } | ||
| const trackPageEvent = useCallback( |
There was a problem hiding this comment.
should it be just trackEvent?
| const { trackPageEvent } = useAnalytics() | ||
|
|
||
| useEffect(() => { | ||
| trackPageEvent({ action: SAFE_NAVIGATION_EVENT, category: 'Collectibles' }) |
There was a problem hiding this comment.
I think we're using category/event/action wrong:
https://github.com/react-ga/react-ga#reactgaeventargs
IMO It should be
category - App Navigation
action - Navigation
label - The name of the view opened
|
|
||
| const toggleSidebar = () => { | ||
| if (!isOpen) { | ||
| trackPageEvent({ action: SAFE_NAVIGATION_EVENT, category: 'SafeListSidebar' }) |
There was a problem hiding this comment.
The last time I used analytics there were no difficulties filtering events written nicely with spaces.
|
Travis automatic deployment: |
|
@mikheevm changes applied |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
2 participants
Closes #1213
Add the following GA events for specific page views:
SafelistSidebar
Assets
Collectibles
Transactions
Apps (name)
Settings