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

Make "Details" search case insensitive #4806

Closed
wants to merge 5 commits into from
Closed

Make "Details" search case insensitive #4806

wants to merge 5 commits into from

Conversation

aerosol
Copy link
Member

@aerosol aerosol commented Nov 12, 2024 •
edited
Loading

This is done by introducing icontains and icontains_not filter operator variants, so that no unexpected behaviour is accidentally triggered.

Dashboard searches for:

  • top/entry/exit pages
  • cities
  • conversions
  • sources (including UTM)
  • props
  • browsers/OS

use icontains explicitly.

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@aerosol aerosol force-pushed the add-case-insensitive-contains-filter-variants branch 2 times, most recently from a0060b8 to 0384cda Compare November 12, 2024 08:23
@aerosol aerosol marked this pull request as ready for review November 12, 2024 08:28
@aerosol aerosol requested a review from a team November 12, 2024 08:28
@aerosol aerosol mentioned this pull request Nov 12, 2024
Closed
@github-actions GitHub Actions
Copy link

Preview environment👷🏼‍♀️🏗️
PR-4806

apata
apata reviewed Nov 12, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@aerosol aerosol force-pushed the add-case-insensitive-contains-filter-variants branch from 1289b1e to 632f18a Compare November 12, 2024 09:06
@aerosol
Make "Details" search case insensitive
9ccfaa0 
This is done by introducing `icontains` and `icontains_not`
filter variants, so that no unexpected behaviour is
accidentally triggered.

Dashboard searches for:

  - top/entry/exit pages
  - cities
  - conversions
  - sources (including UTM)
  - props
  - devices

use `icontains` explicitly.
@aerosol
Update changelog
9826552
@aerosol
Update json schema
63cbd50
@aerosol
Use icontains on browsers and operating systems too
eee9d7a
@aerosol
Update CHANGELOG
12eef49
@aerosol aerosol force-pushed the add-case-insensitive-contains-filter-variants branch from 632f18a to 12eef49 Compare November 12, 2024 09:06
apata
apata approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@apata apata 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.

This looks very good to me.

@aerosol
Copy link
Member Author

aerosol commented Nov 12, 2024

The one thing that comes to mind is to list the new operators

There is a docs PR cross-referenced here

@apata
Copy link
Contributor

apata commented Nov 12, 2024

The one thing that comes to mind is to list the new operators

There is a docs PR cross-referenced here

Yep, good work! Realized as soon as I scrolled up :)

ukutaht
ukutaht reviewed Nov 12, 2024
View reviewed changes
Copy link
Contributor

@ukutaht ukutaht left a comment

Choose a reason for hiding this comment

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

In the Filters RFC we designed for a case sensitivity modifier (see example nr 3) for filters.

I still think that's the preferred solution because it will prevent the explosion of possible filter operators. If we have case-insensitive contains and contains_not then it would also make sense to have a case-insensitive is, is_not, matches, matches_not, and in the future stuff like starts_with, starts_with_not, ends_with, ends_with_not, etc.

@aerosol
Copy link
Member Author

aerosol commented Nov 12, 2024

@ukutaht ah OK, this is much heavier lifting then, potentially involving more URL/linking quirks. I'll drop this for now.

@aerosol aerosol closed this Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ukutaht ukutaht ukutaht left review comments

@apata apata apata approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@aerosol @apata @ukutaht