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.

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

Refactor: Exclude selected prop keys in API instead of in FE #3782

Closed
wants to merge 1 commit into from

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Feb 13, 2024

Solves feedback from original PR: #3719 (comment)

The FE approach would run into problems if you have >25 custom props, so excluding in the BE allows avoiding problems

Feedback from original PR: #3719 (comment)

The FE approach would run into problems if you have >25 custom props, so excluding
in the BE allows avoiding problems
@macobo macobo requested a review from a team February 13, 2024 10:02
@@ -23,7 +23,8 @@ function PropFilterRow({
}) {
function fetchPropKeyOptions() {
return (input) => {
return api.get(apiPath(site, "/suggestions/prop_key"), query, { q: input.trim() })
const exclude = selectedPropKeys.map((key) => key.value)
return api.get(apiPath(site, "/suggestions/prop_key"), query, { q: input.trim(), exclude: JSON.stringify(exclude) })
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of specifying an additional exclude parameter, we should use the formState to update the query object sent to the backend instead. That is similar to all other filter modals.

Right now, if you do the following:

  1. setup: send two events on localhost:
  • mix send_pageview --props "{\"one\":\"1\"}"
  • mix send_pageview --props "{\"two\":\"2\"}"
  • configure these props in Site Settings > Custom Properties
  1. Open the prop filter modal and apply the filter "Property one is 1
  2. Click on the applied filter label on top of the dashboard to open the filter modal again
  3. Click "+ Add another"
  4. Remove the already applied one clicking the red trash icon
  5. Try to select a prop_key

After step 5, two will not be returned anymore, because the suggestions request will be unaware of what is happening in the formState.

Now there's a small blocker here I believe - how do you update the query with the current formState if a prop_key is selected, but the prop_value corresponding to it is empty?

Perhaps we could solve this by disabling (or hiding) the "+ Add another" button when any of the rows is incomplete? Then, the situation where a prop_key is hanging in the formstate without a prop_value would become impossible

@macobo macobo closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants