-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable persons modal on funnel trends #5133
Merged
Merged
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
c6c7379
funnel trends persons call
liyiy 01b5d25
hide nonfunctioning funnel graph display options
liyiy 4245d82
Merge branch 'master' into funnel-trends-persons-modal
liyiy e258ea2
Merge branch 'master' into funnel-trends-persons-modal
liyiy d9c9d60
undo date year conversion
liyiy e5a8dba
avoid forced typing
liyiy d146cae
clean up and add comment
liyiy 484b941
Merge branch 'master' into funnel-trends-persons-modal
liyiy 9ea0ccc
use funnel_viz_type param for funnel graphs
liyiy 6af3060
fix funnel_viz_type param router replacement bug
liyiy 3100a86
Merge branch 'master' into funnel-trends-persons-modal
liyiy 9284c62
fix types and potential display param bug
liyiy 9b319f9
funnel trends should not be an option for non-clickhouse
liyiy 823d974
reuse existing filters
liyiy 4bb60d7
Merge branch 'master' into funnel-trends-persons-modal
liyiy 5dbd0a1
Merge branch 'master' into funnel-trends-persons-modal
liyiy b73a7bb
patch white screen death bug
liyiy 917853b
type predicate
liyiy 9aa4861
undo prettier
liyiy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the addition of this type just confuses things, especially if they're going to be used interchangeably. My preferred approach would be to put everything in
ChartDisplayType
and do away withFunnelVizType
, OR have e.g.ChartDisplayType.FUNNELS
cover everything and haveFunnelVizType
make it more specific.What do you think @mariusandra @alexkim205 ? I don't have a strong opinion here but feel like we should agree on a convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch that, I really think
FunnelVizType
should be removed. We already haveViewType.FUNNELS
to describe funnels overall.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's where the typing will come from for
funnel_viz_type
, see this comment: #5133 (comment)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify,
FunnelVizType
is the type of funnel visualization (since that's separate from overarching visualizations (like Trends or Funnels) )Although, valid question whether ChartFilterProps, the top level interface should expect top level visualisation type or specific-to-funnels visualization type as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, @neilkakkar ,
ViewType
includes Trends, Stickiness, Funnels, Retention, etc. and corresponds to overall type of query.Then
ChartDisplayType
describes, for instance, linear line graph, cumulative line graph, bar chart, pie, etc. and seems like the logical place to distinguish between time to convert, steps, and historical trends as those are "types of visualization".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's also a little confusing to reuse
ChartDisplayType
for something like time conversion because that technically uses theActionsLineGraphLinear
display type and would also probably cause confusion on the frontend if we checked forinsights === FUNNELS && display === ChartDisplayType.ActionsLineGraphLinear
vs justfunnel_viz_type === FunnelVizType.Trends/TimeConversion/whatever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but also agree we should make our chart/display type system easier to navigate in general :o