[WV-1010] Added dataLayer code to Header items [FEEDBACK PROVIDED] #4337
Conversation
There was a problem hiding this comment.
Hi @Zubin-glitch thank you for this pull request. I've added my code review.
| handleTabChange (newValue) { | ||
| handleTabChange (tabValue) { | ||
| // Get the tab info from the mapping | ||
| console.log('handleTabChange ', tabValue); |
There was a problem hiding this comment.
When you make changes based on my comments in the pull request, and update this pull request with the [MERGE READY] title, please comment out all console.log lines you have added.
| @@ -239,10 +243,60 @@ class HeaderBar extends Component { | |||
| if (this.setStyleTimeout) clearTimeout(this.setStyleTimeout); | |||
| } | |||
|
|
|||
| handleTabChange (newValue) { | |||
| handleTabChange (tabValue) { | |||
There was a problem hiding this comment.
I would prefer that you change the variable name of the incoming value to newTabValue over tabValue, since it clarifies that this is a new value we are switching to.
| dataLayer: { | ||
| event: 'headerItemsClick', | ||
| userDetails: { | ||
| voterWeVoteId, |
There was a problem hiding this comment.
Please change these two lines to alphabetical order & directly assign values from VoterStore:
stateCode: VoterStore.getVoterStateCode(),
userCohort: VoterStore.getAnalyticsUserCohort(),
voterWeVoteId: VoterStore.getVoterWeVoteId(),
and remove const voterWeVoteId = and const stateCode = above since we don't need them in local variables.
| pathname: destinationPathname, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Don't forget to include this at the end of this function:
this.setState({ tabsValue: newTabValue });
| console.log('Source page object', currentPage); | ||
|
|
||
| // Map tabValue to destination pathname | ||
| const tabPathMappings = { |
There was a problem hiding this comment.
tabPathMappings will need to be built up in the render method, just above the:
return (
<HeaderBarWrapper
around line 583. You will then pass it into the function handleTabChange. These tab values are dynamic based on the hide/show tab options, like if (isCordova() || inPrivateLabelMode) and else if (voterIsSignedIn).
| @@ -495,7 +558,18 @@ class HeaderBar extends Component { | |||
| howItWorksValue = 99; | |||
| // howItWorksVisible = false; | |||
| squadsValue = 4; | |||
| } else if (friendsVisible) { | |||
There was a problem hiding this comment.
I wouldn't update all the values based only on if friendsVisible is true.
DaleMcGrew
changed the title
DaleMcGrew
changed the title
What github.com/wevote/WebApp/issues does this fix?
Code fix for WV-1010
Changes included this pull request?
Added dataLayer code to handle Header item clicks.
I've removed switch case and utilized lookupPageNameAndPageDict.js functions to fetch pageName and pageType.
Created variables to store tabValues for ballot, candidates and challenges.