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

Add Support for Custom Filters #918

Open
wants to merge 61 commits into
base: main
Choose a base branch
from

Conversation

JettChenT
Copy link
Contributor

Copy link
Collaborator

@milesmcc milesmcc left a comment

Choose a reason for hiding this comment

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

This is very exciting! Looking forward to getting this merged. Some small issues I encountered while testing:

  • It seems like filtering for unset attributes (at least in Incident Type) does not work correctly (a lot of incidents should be showing up in the screenshot below image
  • In the screenshot above, also note that the label in the list is None, but it shows up as [Unset] in the filter listing; could we unify these? (Of course, we need to also allow for filtering where the actual value is None or Unset; tricky design problem!)
  • Right now, when you select a filter in the list, it'll show up as a tag when it is no longer selected regardless of whether you enter a value or not. That might confuse users. Could we instead make it so either 1) the tag does not show up if the user does not enter a value, or 2) the tag shows up the moment the user clicks the value in the filter list?
  • Could we add a placeholder to the geolocation input that says Latitude, longitude? image
  • When no filters match the search, could we display a message that says "No filters found"? Gray text, either text-xl or text-sm based on whatever you think looks better.
  • Let's be sure to use String.to_existing_atom everywhere; using String.to_atom can allow for DOS. For more info, see https://nietaki.com/2018/12/04/string-to-existing-atom-is-a-double-edged-sword/

platform/lib/platform/material/media_search.ex Outdated Show resolved Hide resolved
platform/lib/platform/material/media_search.ex Outdated Show resolved Hide resolved
platform/lib/platform/material/media_search.ex Outdated Show resolved Hide resolved
@JettChenT
Copy link
Contributor Author

Thanks for the review!

  • For the different naming of [Unset] and None, it seems like it is set on purpose for the tag attribute at:
    let name = data.text == "[Unset]" ? "None" : data.text;
    . Should I change "None" to "Unset" here?
  • Applied the other suggestions in the main comment

@milesmcc
Copy link
Collaborator

Ah, I see, the [Unset]/none was not something you introduced. Can we update the way we initialize TomSelect so that it displays the [Unset] tag as None (not set)? If not, then perhaps we remove this special case and just display the value as [Unset] in both the dropdown and in the tag list. Seeing None in the dropdown and [Unset] in the tag list was a bad product decision.

image

You can also ofc keep that out of scope for this PR and make no change, as long as filtering to unset values functions properly.

@JettChenT JettChenT requested a review from milesmcc March 3, 2024 11:51
Copy link
Collaborator

@milesmcc milesmcc left a comment

Choose a reason for hiding this comment

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

So so close! Some small things:

When compiling, I get these errors:

warning: function input_validations/3 required by protocol Phoenix.HTML.FormData is not implemented (in module Phoenix.HTML.FormData.Platform.Material.GenericSet)
  lib/platform/material/generic_set.ex:19: Phoenix.HTML.FormData.Platform.Material.GenericSet (module)

warning: function to_form/4 required by protocol Phoenix.HTML.FormData is not implemented (in module Phoenix.HTML.FormData.Platform.Material.GenericSet)
  lib/platform/material/generic_set.ex:19: Phoenix.HTML.FormData.Platform.Material.GenericSet (module)

Could we resolve these?

Also, when I try to filter based on the date, I'm running into an issue where the date filter doesn't pop up after I select.

CleanShot 2024-03-11 at 14 29 43

Once we get these resolved, I think we'll be able to merge! Thank you again for all your work.

@JettChenT
Copy link
Contributor Author

Thanks for the review! Addressed the issues in the latest commit.

P.S. love the updated date entry UI!

@JettChenT JettChenT requested a review from milesmcc March 15, 2024 00:25
Copy link
Collaborator

@milesmcc milesmcc left a comment

Choose a reason for hiding this comment

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

I think we're good to go! I'll officially merge after our next release (should be within the next week or so) because this is a big PR and I want to make sure we don't accidentally break anything.

@JettChenT
Copy link
Contributor Author

Awesome! Learned a lot working on this PR

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