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

[feature] Compound Filters (input & date) #32

Merged
merged 9 commits into from
Mar 20, 2018

Conversation

ghiscoding
Copy link
Owner

No description provided.

@ghiscoding ghiscoding requested a review from jmzagorski March 18, 2018 05:39
@ghiscoding
Copy link
Owner Author

ghiscoding commented Mar 18, 2018

@jmzagorski
If you have a chance to review by Monday night, else I will merge it anyway. Also note that there's more than just Compound Filters in the PR, I found a few issues with Pagination and Filters at work and a few of these fixes are within the PR. Lastly the first commit is for Compound Filters even though it says fixed untracked files, that was for some files that I wanted to be removed from .gitignore and though I fixed that, I was following this SO question and forgot to name the commit correctly.

Closes #26

@jmzagorski
Copy link
Collaborator

I will review it today. Sorry I have been MIA on some of the tasks I said I was going to do this week. I had to rewrite many of my server Apis.

@ghiscoding
Copy link
Owner Author

No worries, I assumed you were busy, hence if you can't do it then I will merge it. No big issues.

*/
destroy() {
if (this.$filterElm) {
this.$filterElm.off('keyup').remove();
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you need a this.$selectOperatorElm.off('change') here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

oh right, I could do that one too.

Copy link
Collaborator

@jmzagorski jmzagorski left a comment

Choose a reason for hiding this comment

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

This looks real slick 👍, I was able to run it successfully in examples 4 and 5. I have one single comment for you to review and one general question: is your intention when the compound filter goes from a value such as =, >, < to blank that is defaults to equals?

@ghiscoding
Copy link
Owner Author

Not always equals, it depends on the type. If it's a Type String it should be Contains for the rest it will be Equals

@jmzagorski
Copy link
Collaborator

oh okay I see, I did not test that out in example 5 on the Name column

@ghiscoding
Copy link
Owner Author

Hmm I might have forgot to test that portion in a regular grid (without backend service), we should probably test it out in Example 4 or any other. I tested it in OData/GraphQL Backend Services and I have code to handle that. I think the regular grid are handled directly by their filterConditions but I can't remember totally

@ghiscoding
Copy link
Owner Author

It's actually a good thing you talked about this since yes I forgot to test it with a regular grid and it is in fact not working correctly. I just fixed it in my Angular-Slickgrid repo with this commit. I will apply it to the Aurelia repo tonight.

Or perhaps if you have time, I wouldn't mind if you do the changes that you mentioned and the fix for regular grid that I just talked about (on my branch). Or I can try to do them quickly during my lunch time in about 2 hours.

@jmzagorski
Copy link
Collaborator

I can look over your angular commit and see how to fix it.

@ghiscoding
Copy link
Owner Author

I assume that you are able to do the changes on the same branch right? It should be features/compound-filters if I remember correctly

@jmzagorski
Copy link
Collaborator

yup, I have it pulled down on my machine

@jmzagorski
Copy link
Collaborator

@ghiscoding
the same exact change you made in Angular works in Aurelia too. You should now be able to change local filters in example 4 and the data should filter as you expect.

@ghiscoding
Copy link
Owner Author

ghiscoding commented Mar 19, 2018

Yup I try to re-use as much code as possible between the 2 repos 😄
I'll push a release tonight then, thx for the update

EDIT
Oops I found another small issue with the date picker, I use the output type format, to detect if it has an 'h' and if so I add the time picker as well. However I forgot to make it case insensitive so it wasn't picking up 'H' (with Type DateTimeIso which uses an uppercase 'H').

I can update it myself tonight, unless you want to do it, here's the Angular commit

@ghiscoding ghiscoding merged commit 763e766 into master Mar 20, 2018
@ghiscoding ghiscoding mentioned this pull request Mar 20, 2018
13 tasks
@ghiscoding ghiscoding deleted the feature/compound-filters branch March 23, 2018 03:12
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.

2 participants