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.

Sign up for GitHub

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

Jump to bottom

v0.28: Faceting settings #1761

Merged
merged 14 commits into from
Jul 7, 2022
Merged

v0.28: Faceting settings #1761

merged 14 commits into from
Jul 7, 2022

Conversation

guimachiavelli
Copy link
Member

Closes #1704

@guimachiavelli guimachiavelli added this to the v0.28 milestone Jun 28, 2022
@guimachiavelli guimachiavelli linked an issue Jun 28, 2022 that may be closed by this pull request
v0.28: New faceting index setting #1704
Closed
@guimachiavelli guimachiavelli marked this pull request as ready for review June 28, 2022 10:45
maryamsulemani97
maryamsulemani97 suggested changes Jun 28, 2022
View reviewed changes
reference/api/faceting.md Outdated Show resolved Hide resolved
reference/api/faceting.md Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
reference/api/settings.md Show resolved Hide resolved
reference/api/faceting.md Show resolved Hide resolved
guimachiavelli and others added 4 commits June 29, 2022 16:11
@guimachiavelli @maryamsulemani97
Apply suggestions from code review
3ffacec 
Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com>
@guimachiavelli @maryamsulemani97
Update reference/api/faceting.md
bb77d2d 
Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com>
@guimachiavelli
faceting settings: address review feedback
100c17a
@guimachiavelli
faceting settings: address review feedback
456eec5
@guimachiavelli
Copy link
Member Author

Not sure why github actions is complaining. The problematic link is legit. It should be safe to ignore the error.

maryamsulemani97
maryamsulemani97 reviewed Jun 29, 2022
View reviewed changes
Copy link
Contributor

@maryamsulemani97 maryamsulemani97 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We don't mention that the faceting settings are configurable in learn/advanced/filtering_and_faceted_search.md or anywhere. Maybe we could add something under "Using facets" or work on this after v0.28?

reference/api/faceting.md Outdated Show resolved Hide resolved
reference/api/faceting.md Outdated Show resolved Hide resolved
reference/api/faceting.md Outdated Show resolved Hide resolved
@maryamsulemani97
Copy link
Contributor

Not sure why github actions is complaining. The problematic link is legit. It should be safe to ignore the error.

That's because /reference/api/tasks.md#get-task was replaced with /reference/api/tasks.md#get-one-task in #1746

@guimachiavelli guimachiavelli mentioned this pull request Jun 30, 2022
Closed
17 tasks
@guimachiavelli
Copy link
Member Author

guimachiavelli commented Jun 30, 2022 •
edited
Loading

But that's on the base v0.28 branch, not on this one, right? Or do our actions check against the base PR branch? Also, no error locally.

We don't mention that the faceting settings are configurable in learn/advanced/filtering_and_faceted_search.md or anywhere. Maybe we could add something under "Using facets"

Great idea, will do that.

guimachiavelli and others added 2 commits June 30, 2022 12:45
@guimachiavelli @maryamsulemani97
Apply suggestions from code review
4c6a5f9 
Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com>
@guimachiavelli
faceting settings: add note to faceting guide
da8c4a6
maryamsulemani97
maryamsulemani97 approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@maryamsulemani97 maryamsulemani97 left a comment

Choose a reason for hiding this comment

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

Looks good, just one minor change 🧛‍♀️

learn/advanced/filtering_and_faceted_search.md Outdated Show resolved Hide resolved
@maryamsulemani97 maryamsulemani97 mentioned this pull request Jul 4, 2022
Closed
guimachiavelli and others added 2 commits June 30, 2022 13:31
@guimachiavelli @maryamsulemani97
Update learn/advanced/filtering_and_faceted_search.md
c93be15 
Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com>
@maryamsulemani97
Merge branch 'v0.28' into v0.28-faceting-settings
3cf27c6
Kerollmops
Kerollmops requested changes Jul 4, 2022
View reviewed changes
reference/api/faceting.md Outdated Show resolved Hide resolved
@maryamsulemani97 @guimachiavelli
v0.28: Add faceting to core concepts+index settings (#1767)
be6cd5c 
* add faceting to index settings+core concepts

* Update learn/configuration/settings.md

* Update settings.md

* Update learn/core_concepts/indexes.md

* Update learn/core_concepts/indexes.md

Co-authored-by: gui machiavelli <gui@meilisearch.com>

Co-authored-by: gui machiavelli <gui@meilisearch.com>
gmourier
gmourier requested changes Jul 6, 2022
View reviewed changes
Copy link
Member

@gmourier gmourier left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@guimachiavelli If you decide to explain how facets values are sorted, I made a little suggestion to make it clearer.

Other than that everything seems clear to me ✨

reference/api/faceting.md Outdated Show resolved Hide resolved
@guimachiavelli @gmourier
Update reference/api/faceting.md
8fa84da 
Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com>
gmourier
gmourier approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@gmourier gmourier left a comment

Choose a reason for hiding this comment

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

LGTM!

@guimachiavelli guimachiavelli requested a review from Kerollmops July 6, 2022 11:49
Kerollmops
Kerollmops approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

LGTM thank you very much!

This was referenced Jul 6, 2022
Merged
Merged
@guimachiavelli
Copy link
Member Author

bors merge

bors bot added a commit that referenced this pull request Jul 7, 2022
@bors @guimachiavelli @maryamsulemani97
Merge #1761
5dd44ec 
1761: v0.28: Faceting settings r=guimachiavelli a=guimachiavelli

Closes #1704 

Co-authored-by: gui machiavelli <hey@guimachiavelli.com>
Co-authored-by: gui machiavelli <gui@meilisearch.com>
Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jul 7, 2022

Build failed:

@guimachiavelli guimachiavelli merged commit 2eda982 into v0.28 Jul 7, 2022
@guimachiavelli guimachiavelli deleted the v0.28-faceting-settings branch July 7, 2022 10:23
@bidoubiwa bidoubiwa mentioned this pull request Aug 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maryamsulemani97 maryamsulemani97 maryamsulemani97 approved these changes

@gmourier gmourier gmourier approved these changes

@Kerollmops Kerollmops Kerollmops approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.28
Development

Successfully merging this pull request may close these issues.

v0.28: New faceting index setting
4 participants
@guimachiavelli @maryamsulemani97 @Kerollmops @gmourier