Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(data-warehouse): make separate property type in taxonomic filter for data warehouse person properties #21169

Merged
merged 11 commits into from
Apr 2, 2024
Merged

feat(data-warehouse): make separate property type in taxonomic filter for data warehouse person properties #21169

merged 11 commits into from
Apr 2, 2024

Conversation

EDsCODE
Copy link
Member

@EDsCODE EDsCODE commented Mar 26, 2024

Problem

Changes

  • do it again just keep the properties as a separate type in the dropdown

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 26, 2024 •
edited
Loading

Size Change: 0 B

Total Size: 824 kB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 824 kB

compressed-size-action

@EDsCODE EDsCODE requested review from mariusandra and Twixes March 27, 2024 03:08
@EDsCODE EDsCODE changed the title feat(data-warehouse): make separate property type in popup for data warehouse person properties feat(data-warehouse): make separate property type in taxonomic filter for data warehouse person properties Mar 27, 2024
mariusandra
mariusandra approved these changes Mar 27, 2024
View reviewed changes
Copy link
Collaborator

@mariusandra mariusandra left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I only wonder if it's worth passing the table prop around everywhere, or if it could somehow be inlined into the value.

@EDsCODE
Copy link
Member Author

EDsCODE commented Mar 28, 2024

Looks good. I only wonder if it's worth passing the table prop around everywhere, or if it could somehow be inlined into the value.

changed it around to inline the value. I also just opened up a new property type. This keeps everything really clear right now so there's not an implicit thing to worry about with the existing person properties

mariusandra
mariusandra approved these changes Mar 28, 2024
View reviewed changes
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't run, but code looks good, and clean! 👍

frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx
@@ -225,6 +228,16 @@ export const taxonomicFilterLogic = kea<taxonomicFilterLogicType>([
getPopoverHeader: () => 'Data Warehouse Column',
getIcon: () => <IconServer />,
},
{
name: 'Data Warehouse Person Properties',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a long name 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

External Person Properties?

@EDsCODE EDsCODE merged commit d9da73e into master Apr 2, 2024
84 checks passed
@EDsCODE EDsCODE deleted the dw-data-warehouse-person-properties-2 branch April 2, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neilkakkar neilkakkar neilkakkar left review comments

@mariusandra mariusandra mariusandra approved these changes

@Twixes Twixes Awaiting requested review from Twixes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EDsCODE @mariusandra @neilkakkar