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

873 full text search #1000

Merged
merged 5 commits into from Mar 13, 2019

Conversation

Projects
None yet
3 participants
@hukka
Copy link
Collaborator

hukka commented Mar 12, 2019

  • resolve nulls being passed into props
  • style tweaking
  • maybe we don't want to search from all translations?
  • fix tests

hukka added some commits Mar 11, 2019

Update clojurescript
This fixes false positives of "undeclared var"
@hukka

This comment has been minimized.

Copy link
Collaborator Author

hukka commented Mar 12, 2019

Works for example by searching "developer draft 2019 elfa"

Show resolved Hide resolved src/cljs/rems/table.cljs Outdated
Show resolved Hide resolved src/cljs/rems/table.cljs
Show resolved Hide resolved src/cljs/rems/table.cljs
Show resolved Hide resolved src/cljs/rems/table.cljs Outdated
Show resolved Hide resolved src/cljs/rems/table.cljs Outdated
Show resolved Hide resolved src/cljs/rems/table.cljs Outdated
Show resolved Hide resolved src/cljs/rems/table.cljs Outdated
Show resolved Hide resolved src/cljs/rems/table.cljs Outdated
Show resolved Hide resolved src/cljs/rems/table.cljs Outdated
(defn match-filters? [column-definitions filters item]
(every? (fn [filter-word]
(match-filter? column-definitions filter-word item))
(str/split filters #"\s+")))

This comment has been minimized.

@Macroz

Macroz Mar 12, 2019

Collaborator

I don't think this is a good place for the str/split. it should be done earlier so that filters can be a seq already.

This comment has been minimized.

@hukka

hukka Mar 13, 2019

Author Collaborator

At most it could be moved one level up to apply-filters, which doesn't make much of a difference. Any higher and we are in representational logic that shouldn't really care how filter logic wants to interpret the filter commands.

This comment has been minimized.

@Macroz

Macroz Mar 13, 2019

Collaborator

I think str/split belongs to the representational layer since it's parsing user input and not in the logic layer here. That's why I suggest that you move the parsing up to the border where the input callback is. Internally you can store the filters in a more optimal format for search and do less processing every call.

This comment has been minimized.

@Macroz

Macroz Mar 13, 2019

Collaborator

I think it might make sense to refactor this later anyway since there is very little value in having :filterable? and :sortable? separately compared to only specifying :filter-value and :sort-value where necessary.

This comment has been minimized.

@luontola

luontola Mar 13, 2019

Collaborator

:filter-value and :sort-value fall back to :value, so :filterable? and :sortable? exist to disable the default behavior.

This comment has been minimized.

@Macroz

Macroz Mar 14, 2019

Collaborator

Yes but that is also suspect.

@hukka hukka force-pushed the 873-full-text-search branch from ec6e48a to 9020f2e Mar 13, 2019

@hukka hukka force-pushed the 873-full-text-search branch from 9020f2e to 357140b Mar 13, 2019

@hukka hukka changed the title WIP 873 full text search 873 full text search Mar 13, 2019

@hukka hukka requested a review from Macroz Mar 13, 2019

@Macroz

Macroz approved these changes Mar 13, 2019

@Macroz Macroz merged commit 711085e into master Mar 13, 2019

6 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: cloverage Your tests passed on CircleCI!
Details
ci/circleci: doo Your tests passed on CircleCI!
Details
ci/circleci: ok Your tests passed on CircleCI!
Details
ci/circleci: war Your tests passed on CircleCI!
Details
ci/circleci: without-db Your tests passed on CircleCI!
Details

@Macroz Macroz deleted the 873-full-text-search branch Mar 13, 2019

@hukka hukka referenced this pull request Mar 14, 2019

Open

vTHL3: Full text search #873

1 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.