feat(web-analytics): Hide persons for web analytics #18445
Conversation
| dashboardItemId: `new-AdHoc.${key}`, | ||
| query, | ||
| setQuery, | ||
| ...(context?.insightProps || {}), |
There was a problem hiding this comment.
Flagging this change - I don't have enough context to be 100% certain this is safe
Update: this change no longer exists
There was a problem hiding this comment.
I'm not sure either... I see the main change is that you added three things to this insightLogicProps:
hidePersonsModal?: boolean
suppressSessionAnalysisWarning?: boolean
chartRenderingMetadata?: ChartRenderingMetadataThe existing insight logic props have to do with just the insight ID or cached results, these are very specific toggles for some specific insights. My first reaction is that they feel out of place. Since they do affect how the insight is displayed, should/could they just be part of the query node itself?
There was a problem hiding this comment.
Do you think they make more sense as part of QueryContext? I initially started there, essentially being consistent with the UI-only props for DataTables. Otherwise they could potentially go on InsightQueryBase, if we wanted them as part of the actual query node. I could also put them on any of the children of that (like e.g. TrendsQuery), but I don't like that as much, because they are more about the chart than the insight type.
There was a problem hiding this comment.
Hmm I guess putting them on the InsightVizNode could also work
There was a problem hiding this comment.
Toggling these will not affect the fetched data, so the InsightVizNode seems like the better option indeed.
There was a problem hiding this comment.
Ah the chartRenderingMetadata contains a function so can't go inside the query node
There was a problem hiding this comment.
Could that be passed in the context for the <Query> tag?
There was a problem hiding this comment.
Yeah, we currently don't pass this into the Trends component, but some of the ActionX components can receive it as a prop, so I can change how this is wired up.
My plan is to split this, with the chart rendering functions moving to the context, and the other ones moving to the InsightViz node.
Sent from my iPhone
There was a problem hiding this comment.
This re-wiring is done, I only touched the TrendInsight component and not the others e.g. FunnelInsight but it would be very simple to wire those up the same way if needed later
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
robbie-c
force-pushed
the
hide-persons-for-web-analytics
branch
from
a704922 to
58c2840
Compare
robbie-c
changed the title
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
robbie-c
force-pushed
the
hide-persons-for-web-analytics
branch
from
50f560a to
6cd4140
Compare
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
robbie-c
force-pushed
the
hide-persons-for-web-analytics
branch
2 times, most recently
from
5e2d9f3 to
8c2aded
Compare
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
robbie-c
force-pushed
the
hide-persons-for-web-analytics
branch
from
8546209 to
39a9ce1
Compare
robbie-c
force-pushed
the
hide-persons-for-web-analytics
branch
from
39a9ce1 to
c0f4ad1
Compare
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
robbie-c
force-pushed
the
hide-persons-for-web-analytics
branch
from
184d4ae to
501bf9a
Compare
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Problem
Web analytics won't have the concept of persons, so hide them from the UI.
Additionally, filtering by country could be easier
Changes
Make the world map clickable to filter for users of that country only, by adding chart-specific rendering options. Right now these only exists for the world map but could be expanded.
How did you test this code?
Ran manually