Skip to content

[WIP] Column search #221

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

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open

Conversation

mklein994
Copy link
Contributor

@mklein994 mklein994 commented Apr 18, 2018

Add search inputs for each column header.

Things to do before merging

  • Implement in GtCustomComponentFactory
  • Move column search demo module to src/app/demos/ src/app/examples/search/
  • Make styling more consistent with the rest of the app
  • Filter rows by each column's search box (if enabled)
  • Highlight search matches
  • Handle (keyup) event asynchronously (maybe?)
  • Ability to show only null values Filter column to show only values set to null mklein994/angular-generic-table#1
  • Search by types other than string Search using types other than string mklein994/angular-generic-table#2
  • Split search terms like how it's done in global search
    • For example: searching for smith brian under a 'name' field, where first and last names are joined with a space, should find "Brian Smith" if it exists. If the query is wrapped in double quotes, it's counted as one word, so a search "smith brian" would not find "Brian Smith".
    • TODO: what about case sensitivity?

Closes #99

@mklein994
Copy link
Contributor Author

mklein994 commented Apr 18, 2018

I've labeled commits that shouldn't be merged into master with DO NOT MERGE:, to make them easier to find later. These include things like changing travis' build scripts to be specific to this PR, and other minor things. When this PR is ready, I'll rebase those out.

EDIT: I've cleaned it up to remove those commits. This makes the PR represent more closely what would happen if it were to be merged.

@mklein994 mklein994 force-pushed the column-search branch 2 times, most recently from 67f1699 to 3db435a Compare April 18, 2018 15:09
@mklein994
Copy link
Contributor Author

For some reason, I can't seem to get it to work on angular 4, but if I upgrade to angular 5 it works. ng-packgr is failing silently when running npm run build:core.

I'm going to clean this branch up so that others can contribute to it more easily. (That way I can rebase only when master get's updated).

@hjalmers
Copy link
Owner

Hi @mklein994 nice work, I'm also in the middle of refactoring the examples and how they are generated as that's one of the main things that have prevented me from bumping the code to angular 5, once I'm done with that it should be easier for others to contribute I hope. I haven't done anything changes to the lib so it should be pretty easy to merge, feel free to check it out if you like: https://github.com/hjalmers/angular-generic-table/tree/feat/angular-5-update

I've just added one example using the new "format" which is available under '/demo/basic'.

@mklein994
Copy link
Contributor Author

@hjalmers I left a comment over here about some of those updates.

@mklein994
Copy link
Contributor Author

I rebased onto master, updating each commit to match the new style.

@mklein994
Copy link
Contributor Author

Rebase onto master (cypress tests).

@mklein994
Copy link
Contributor Author

mklein994 commented Apr 24, 2018

@hjalmers OK: so this works... when the table is small. I took your advice wrote it using pipes, because that seemed to be the simplest option to extend it. Everything is performed synchronously, so when I enabled searchBox for some fields in the advanced demo, performance was taking a big hit, as you predicted. (It was also failing because I haven't implemented the search or value transformations yet).

If I were to re-implement this using observables and triggering it asynchronously, what would need to be updated?

Edit:

Scratch that: it doesn't work 😕. I'm going to keep working on it, but the filter is duplicating items, and not filtering when it should. I'm still curious about how to implement this as an observable.

@hjalmers
Copy link
Owner

Yeah, I first wrote the search function for AngularJs which had a lot of issues with performance due to two-way binding so I tried to minimize the amount of operations performed and just trigger one change and it becomes even trickier when involving multiple columns. I think a lot of the code from the global search could be reused however, but instead of merging all the columns into one string, it could go through the data one column at a time and only go through columns that are included in the search "object". The next iteration should only have to go through the rows that pass the first column check and so on, it shouldn't have to have have that big impact on performance.

It would be a lot nicer to use observables however but I think it would be hard to use observables just for this with out rewriting the other pipes too, I think it would be better to start from scratch and think about the flow which should be something like this:

  1. filter
  2. global search
  3. column search (might be performed at the same time as global search)
  4. sort
  5. chunk

Perhaps starting out with a separate component that uses the same configuration and just re-add one feature at a time and use observables for everything.

I thought I'd try and draw a sketch of the flow using draw.io or something just to make the architecture clearer but that will have to wait a while I'm afraid as I'm going away on a holiday soon and won't be back until mid may. But if you have any suggestions feel free to share them, I'm might not be able to commit any code for a while but I should be able to check my mail from time to time and merge minor pull requests.

@mklein994
Copy link
Contributor Author

OK thanks for the heads-up.

This enables a search box for that header.

A search field in the generic-table component was also added.
This will hold the object keys as properties, and strings as their
values. This may not be the best way to implement it, because the
interface just has an [index signature][1] for the column names, but it
seems to work for now. Hopefully there just get referenced by objectKey.

[1]: https://www.typescriptlang.org/docs/handbook/interfaces.html#excess-property-checks
WIP; doesn't actually filter rows yet, and it kinda spams the debug log.
This allows angular to detect a change in the pipe.
Replaces the column search array every time a column search box is
updated.

Works so far as long as the field values can be cast as strings.

Also adds the column search pipe to the other chains that include
gtSearch and gtFilter pipes.
Empty column search strings are ignored. This means that the filter does not
apply to that column.
mklein994 and others added 27 commits May 3, 2018 09:40
Better than passing $event.

Also remove unused @ViewChild('myTable') reference.
Split into smaller functions to make it more manageable.
Fields with null values are included (for now) only when the search box
for that column is empty.

This fixes a bug where searching in a column that contains null values
would crash.
Before, when a field defined a search function, the transformation
persisted beyond the column search pipe.

Now, the transformation is only done to confirm a match; the original
value for that field is returned.
Resolved conflict by adding a call to toggleAllRows() in the gt-checkbox
element.
Resolved conflicts by moving the generic table template changes to the
new file.
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.

3 participants