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

fix: filterQueryOverride provide all search values #1611

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

zewa666
Copy link
Contributor

@zewa666 zewa666 commented Jul 18, 2024

meh there was an issue where suddenly on multi-selects I didn't get the value of the second and onwards selection

Copy link

stackblitz bot commented Jul 18, 2024

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

@zewa666
Copy link
Contributor Author

zewa666 commented Jul 18, 2024

hmmm do you think its ok to change the signatures for graphql as well @ghiscoding?

@ghiscoding
Copy link
Owner

ghiscoding commented Jul 18, 2024

oh I see, that was a mistake in the implementation, yes it should be plural since we always deal with a searchTerms array and yes we should update the GraphQL and anywhere else. This feature is relatively new, so it should be ok to update it (with a warning message in the release section) and that seems to be why the build failed in your PR as well

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (e34971d) to head (4ae38b9).
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1611     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         198     198             
  Lines       21795   21796      +1     
  Branches     7302    7303      +1     
========================================
+ Hits        21734   21735      +1     
+ 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

There's another line to change it seems

Error: src/examples/example10.ts(216,79): error TS2339: Property 'searchValue' does not exist on type 'BackendServiceFilterQueryOverrideArgs'.

@zewa666
Copy link
Contributor Author

zewa666 commented Jul 18, 2024

yeah just saw it ... not sure if I fixed it properly though as the Graphql example doesn't show any results

@ghiscoding
Copy link
Owner

ghiscoding commented Jul 18, 2024

yeah just saw it ... not sure if I fixed it properly though as the Graphql example doesn't show any results

the GraphQL example only shows the query string (because I don't have a dedicated GraphQL server or fake data like OData), so technically just take a look at the query to see if it work as intended or not. That is also what the E2E tests are testing

@ghiscoding
Copy link
Owner

looks good, I'll go ahead and merge.

Have you seen my new draft PR #1609? I'm not sure if I'll wait for this PR to be finished before pushing a new release or simply go without it

@ghiscoding ghiscoding merged commit c03b863 into ghiscoding:master Jul 18, 2024
6 checks passed
@zewa666
Copy link
Contributor Author

zewa666 commented Jul 18, 2024

yeah I saw it, didnt have time to properly check it out, plan to do so at home. sounds definitely interesting if only for feature comparison reasons to the big player. I personally feel though that virtual scrolling has far more drawbacks compared to pagination in server-scenarios. nevertheless, I'll give it certainly a try.

next week I'll finally start to combine AI together with the grid and I'm super excited about the potential use cases. the slickgrid project at work made a huge impact so people feel fine to invest a more time 🎉

edit: with regards to new release, no pressure, I've monkey patched my way until the new release is out and whenever that happens I just need to delete those patches.

@ghiscoding
Copy link
Owner

next week I'll finally start to combine AI together with the grid and I'm super excited about the potential use cases. the slickgrid project at work made a huge impact so people feel fine to invest a more time 🎉

glad to hear this, I rarely hear good stories, I usually hear more about the issues haha

For the release, I think there's enough to go ahead with a new one this weekend and I also want to take my time for that Infinite Scroll feature and it will take time to add tests and also add the feature for GraphQL as well (currently testing with OData since I have test data) and maybe infinite scroll for local dataset (not sure yet)

@zewa666
Copy link
Contributor Author

zewa666 commented Jul 18, 2024

wont say no to that ;)

to give you an idea where new potential features might go with our app (just hacked this together in an hour with a local Mistral LLM and context of the whole row)

grid_ai_yeah

@ghiscoding
Copy link
Owner

So your last column as a Formatter with a button for the AI, is that it? I haven't much of AI stuff at my work yet but I should dig more into this new tech, it's definitely the future going forward. So is that AI button reading the new row data you've input and provide suggestions or whatever else with that data? It seems quite interesting. It reminds me a bit when I added a translate button for users with different language and they wanted to see it translated on the fly

Side note, my draft PR is also Example 27, so I'm not sure if you wanted to add this new example to my repo but if so, we'll for sure conflict :D

@zewa666
Copy link
Contributor Author

zewa666 commented Jul 18, 2024

dont worry I just took whatever number I saw free. Your example with on the fly translation is actually quite close to it. What one does with it depends on how the prompt is built. In my case I've just instructed it to improve the failure description by using so far text + title.

the interesting part is going to be able to create the prompt along with a template, which allows interpolating the active or other rows to provide context.

I'll play with it next week and let you know more

@ghiscoding ghiscoding changed the title fix: provide all search values fix: filterQueryOverride provide all search values Jul 20, 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

3 participants