Fix analytics setup flow not rendering for accounts that only share views and have no associated analytics account. #53
Conversation
… to prevent empty render
ivankristianto
left a comment
ivankristianto
left a comment
There was a problem hiding this comment.
Code looks good and It works.
|
|
||
| // Accounts will always include Set up New Account option unless existing tag matches property. | ||
| if ( ( 1 >= accounts.length && ! existingTag ) || ( 0 >= accounts.length && existingTag ) || setupNewAccount ) { | ||
| if ( 0 >= accounts.length ) { |
There was a problem hiding this comment.
@lucymtc Can you provide some context on why exactly you changed this the way you did, with background on the individual checks?
I would like to have more insight since the previous conditions were quite complex, and now those were removed, so I want to make sure we don't run into regressions.
There was a problem hiding this comment.
@felixarntz in property and profile dropdown we have the options "Set a new property" and "Set a new profile", we used to have also "Set a new account" in account dropdown only when existingTag was false. so 1 >= accounts.length was actually taking into count "Set a new account" option.
This option was removed from the dropdown, so for users that only have 1 analytics account 1 >= accounts.length && ! existingTag returns true and it shouldn't, this should only return true now when accounts.length = 0
There was a problem hiding this comment.
Thanks for the response. Wouldn't we need to keep the setupNewAccount condition though?
There was a problem hiding this comment.
yes, that can be removed. I have pushed an update
There was a problem hiding this comment.
Great, thanks for this! Let's make sure we always clean up after ourselves when making such changes, otherwise we'll end up with a cluttered codebase, and in the future nobody will know why that property exists.
ivankristianto
left a comment
ivankristianto
left a comment
There was a problem hiding this comment.
Code looks good to me.
3 participants
Summary
This PR can be summarized in the following changelog entry:
Fixes #11
Relevant technical choices
Checklist: