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

feat(filters): add StartsWith/EndsWith (a*z) filter combo #1530

Merged
merged 4 commits into from
May 16, 2024

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented May 16, 2024

TODOs

  • add unit tests
  • add E2E tests
  • requires more manual tests in UI
  • not sure what happens when using something like Ta*10*, that probably won't work, should we support that?
    • returns same result as Ta*10 but that's ok, I won't support more complex scenarios like this one.
    • we'll support only 1 * symbol

brave_40cP5F4RL9

Copy link

stackblitz bot commented May 16, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 16, 2024

@zewa666 first PR to add a new OperatorType.StartsWithEndsWith (alias to a*z string), I've done some quick tests and it seems to work as intended for a local JSON dataset. I assume that you could look at the OData/GraphQL in a separate PR 😉 It's looking good so far.

Things to note though, I don't think this new operator should be added to the compound filters, because the only way to say that it's a startsWith/endsWith string is really to write ab*xyz (including the *) so I don't see the point of the compound filter operator in that case...

Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (828eb8a) to head (4bf696f).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1530     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         198     198             
  Lines       21621   21633     +12     
  Branches     6951    7089    +138     
========================================
+ Hits        21560   21572     +12     
+ Misses         61      55      -6     
- Partials        0       6      +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 16, 2024

@zewa666 I'm not sure what would happen if we type Ta*10*, that probably won't work, however should we really go that deep and support that case? I think that I'll just skip that use case and only support one * char at a time for these 3 (previously 2) scenarios and be done with it

  1. Ta* starts with
  2. *100 ends with
  3. Ta*100 starts with + ends with (new)
  • Ta*100* this will probably return nothing

@zewa666
Copy link
Contributor

zewa666 commented May 16, 2024

to be honest I'm not sure. on one hand its overkill for a simple client side filter. on the other hand since SQL supports this natively with LIKE it could cause a
mismatch between client/server filtering. but that said its also a very narrow usecase for a db driven backend. who says a graphql service needs to grab data from a DB.

in all honesty, I think you went waaaay over the stretch goal to answer the original SO question, so I'd keep it at the state you have and call it a day. It can always be improved later on.

BTW, do you have a rough estimate on when you're planing to release the new major both for universal and angular, absolutely no pressure here just curious? I wonder whether I should wait for that or do an interim update for my app. heck you're so damn fast with new releases I'm already 3-4 behind 🤣

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 16, 2024

Yeah I also think it's probably overkill to go that deep into a simple input text filter and yeah I know I went over the top with the SO question but I was kind of curious at the same time and thought we should support at least a*z, so that was a good exercise (even though it was much longer than I expected lol).

BTW, do you have a rough estimate on when you're planing to release the new major both for universal and angular, absolutely no pressure here just curious? I wonder whether I should wait for that or do an interim update for my app. heck you're so damn fast with new releases I'm already 3-4 behind 🤣

I'm waiting for Angular 18 which is scheduled for next week, I hope that they won't have any delay though (which happened in the past). Since I already release 5.0 here and the new stuff will be under 5.1, I'm thinking of pushing 2 successive releases of Angular-Slickgrid to be on the same minor as Slickgrid-Universal (unless I figure out how to force release-it directly to v8.1, I now use GitHub Action to release so that it comes with --provenance enabled but the versions are auto-detected via conventional changelog, so anyway I could look into a prompt release if possible).

image

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 16, 2024

also the reason I'm waiting for Angular 18 is not just because it's nearby but mainly because I don't expect to ship that many (if any) major versions in the future (since I consider to be pretty much feature complete and done after 7 years, it's about time), so I think it's better to stick with latest Angular if possible (that is why I decided to wait few weeks for Angular latest version)

There's a new issue opened in SlickGrid asking for Pivot Table, but personally I think that's over the top and I won't work on that kind of things when user can simply Export to Excel and execute the Pivot Table in Excel. It's always the fact that when you give a candy, not only do they ask for more but they often ask for the entire cookie jar for free of course 🤣

Anyway, I like that you made so many contributions to the project, there aren't that many who take the time to contribute 🥇

@zewa666
Copy link
Contributor

zewa666 commented May 16, 2024

glad to hear you're happy with your journey. Pivot tables indeed would open up a whole new array of issues but never could fully compete with PowerPivot so no point in that. Besides having focused on the OData provider I at least can generate the metadata file and feed the same source directly to PP. Best of both worlds 💪 to be honest your intro to the OData backend opened up a whole new range of possibilities which make the overall development so much better. Just today I've updated as an example the ngSelect custom filter to allow selecting multiple filter items and it was mereley switching the default operator from eq to in and boom it just works. still mindblown about that one

I plan to focus next week more on our custom foreign key editor/filter/formatter. while doing so and trying out new apis its easier to spot a couple little gotchas and fixing them on-the-fly is a win-win.

let me just say again that I'm super grateful for your determination and your focus on quality. its a joy working and contributing to such a great OSS project and learning new things while doing so

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 16, 2024

indeed it's a long journey and I learned quite a lot, also happy to hear that these backend services were not just useful but also brought auto-magic in some ways hehe. 🐰

@ghiscoding ghiscoding merged commit 51560aa into master May 16, 2024
7 checks passed
@ghiscoding ghiscoding deleted the feat/starts-ends-with branch May 16, 2024 23:37
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