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

review filtering guide #2061

Merged
merged 17 commits into from
Feb 16, 2023
Merged

review filtering guide #2061

merged 17 commits into from
Feb 16, 2023

Conversation

maryamsulemani97
Copy link
Contributor

@maryamsulemani97 maryamsulemani97 commented Dec 27, 2022

closes #1902, #860, #837, and #1850

  • moves all operators to a new heading
  • removes SQL syntax and combines it with the curl code samples
  • replaces amazon and imdb images for facets with meilisearch demo images
  • removes unused images in .vuepress/public/faceted-search
  • remove redundant code samples

  • Improve coherence between dataset and examples
    • A sample document was added as part of Add code samples for sorting/filtering nested fields #2050 with rating and director fields
    • This PR uses movie_ratings instead of movies to clarify that this is a separate dataset
      • faceted_search_facets_1 still uses movies as this code sample is used in the API reference as well

@maryamsulemani97 maryamsulemani97 marked this pull request as ready for review January 2, 2023 07:44
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
Copy link
Member

@guimachiavelli guimachiavelli left a comment

Choose a reason for hiding this comment

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

I think we're almost there, but I have an extra comment regarding document structure.

In "Filter basics" we introduce two important terms: filter expressions and conditions. Though we give extensive definitions of conditions, filter expressions are only briefly defined mid-sentence on L#60. Since we use the term "filter expressions" throughout the document, I'm afraid inattentive users might have a hard time finding that definition because it's so easily missed—a quick scroll will not reveal any relevant headings and they will have to do a cmd+f and type "expression", possibly having to skip a few matches until they find a satisfactory explanation.

I can think of at least two ways of helping these users:

  1. having a short section named "Filter expressions" with a definition and a code sample. For example:
### Filter expressions

Filter expressions combine multiple conditions with operators. They are always written in the `condition1 OPERATOR condition2` format:

```
genre = horror AND release_year > 2001
```
  1. adding a brief definition of filter expressions to "Combining filter expressions". Could also involve changing this heading to something like "Combining conditions into filter expressions" (probably not a good title, but you get the idea)

I think #1 is simpler, but might be overkill.

learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
@maryamsulemani97
Copy link
Contributor Author

In "Filter basics" we introduce two important terms: filter expressions and conditions. Though we give extensive definitions of conditions, filter expressions are only briefly defined mid-sentence on L#60. Since we use the term "filter expressions" throughout the document, I'm afraid inattentive users might have a hard time finding that definition because it's so easily missed—a quick scroll will not reveal any relevant headings and they will have to do a cmd+f and type "expression", possibly having to skip a few matches until they find a satisfactory explanation.

I renamed the heading "Combining filter expressions" to "Filter expressions". That should make it more visible:
Screen Shot 2023-01-16 at 12 31 21

I didn't add "They are always written in the condition1 OPERATOR condition2 format:" because you can only use AND or OR. Both of them are mentioned in the first sentence of the paragraph and are subheadings

@guimachiavelli
Copy link
Member

@dichotommy, you mentioned you wanted to review this. Will you have time for it?

Copy link
Contributor

@dichotommy dichotommy left a comment

Choose a reason for hiding this comment

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

Looks pretty good! I'm submitting half my review now so you can get started on it, will be back later to review the rest.

learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dichotommy dichotommy left a comment

Choose a reason for hiding this comment

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

On y va! 🚀

@maryamsulemani97
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Feb 16, 2023
2061: review filtering guide r=maryamsulemani97 a=maryamsulemani97

closes #1902, #860, #837, and #1850

- moves all operators to a new heading
- removes SQL syntax and combines it with the curl code samples
- replaces amazon and imdb images for facets with meilisearch demo images
- removes unused images in `.vuepress/public/faceted-search`
- remove redundant code samples
<hr>

- Improve coherence between dataset and examples
  - A sample document was added as part of  #2050 with `rating` and `director` fields
  - This PR uses `movie_ratings` instead of `movies` to clarify that this is a separate dataset
     - `faceted_search_facets_1` still  uses `movies` as this code sample is used in the API reference as well

Co-authored-by: Maryam Sulemani <[email protected]>
Co-authored-by: Maryam <[email protected]>
Co-authored-by: maryamsulemani97 <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 16, 2023

This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried.

Additional information:

Response status code: 422
{"message":"1 review requesting changes and 1 approving review by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

@maryamsulemani97
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 16, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants