Enhancement/3291 property selection updates #3384
Conversation
|
Size Change: +941 B (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks @eugene-manuilov – this looks good overall and is close to ready, just a few suggestions and questions (including one for @felixarntz)
assets/js/modules/analytics/components/common/AccountSelect.test.js
Outdated
Show resolved
Hide resolved
tests/e2e/specs/modules/analytics/setup-with-account-no-tag.test.js
Outdated
Show resolved
Hide resolved
| * @param {Array.<string>|string} url A list of URLs or a signle URL to match properties. | ||
| * @return {Object|null} A property object if found. NULL if nothing is found. | ||
| */ | ||
| *matchPropertyByURL( properties, url ) { |
There was a problem hiding this comment.
This doesn't need to be a generator 😄
| *matchPropertyByURL( properties, url ) { | |
| matchPropertyByURL( properties, url ) { |
@felixarntz is there any reason why this can't be a utility instead since it doesn't use the datastore at all?
I understand it mirrors the action in the GA4 store, but I don't think we should add it for that reason alone since these stores will always have some methods the other does not (e.g. profile/webdatastream).
There was a problem hiding this comment.
@aaemnnosttv Fair point. I don't have a strong opinion on this - we can make this a utility function if preferred.
There was a problem hiding this comment.
Let's do it in a separate ticket then? Since we already have similar functionality in the analytics-4 module.
There was a problem hiding this comment.
This doesn't need to be a generator
It needs to be a generator because otherwise, we would need to return a regular redux action object.
There was a problem hiding this comment.
This doesn't need to be a generator
It needs to be a generator because otherwise, we would need to return a regular redux action object.
Oops, yeah you're right, although I don't think we use actions in this way anywhere else except for those that yield controlled actions since these must be a generator. I suppose it's okay for now then, but this as a generator only for this reason when it doesn't yield anything is an odd detail that someone else might see and "fix" which breaks it. For this reason I'd prefer we extract it sooner than later. If you feel that this would take too much time right now then let's clearly document this as an intentional requirement of the function.
we already have similar functionality in the analytics-4 module.
The use in analytics-4 looks okay to me since it's actually yielding actions although maybe that could be cleaned up with a utility in the future.
There was a problem hiding this comment.
@aaemnnosttv sounds good to me. Updated.
…roperty-selection-updates.
…ion from the util package.
Summary
Addresses issue #3291
Relevant technical choices
Checklist