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

v0.28: Pagination guide #1753

Merged
merged 11 commits into from
Jul 7, 2022
Merged

Conversation

guimachiavelli
Copy link
Member

@guimachiavelli guimachiavelli commented Jun 22, 2022

Closes #561

@guimachiavelli
Copy link
Member Author

@dichotommy: I know you're short on time, so these are the areas I'm mostly interested in receiving feedback:

  1. Structure: is the structure of intro > limitations > recommended solution > workarounds satisfactory? Will we help users who are having problems with building a page selection UI?
  2. Terminology: I often use "page selection" and "page selector" to refer to the classic pagination UI. Is it clear what I'm talking about?
  3. Length: I feel this is still too big, but perhaps I'm wrong?
  4. Code samples: are the ones we currently have enough? Do they clearly illustrate our points?

Of course, any feedback is appreciated, especially if you identify any other major areas of concern I'm not aware of because I'm too much inside the subject atm.

@guimachiavelli guimachiavelli added this to the v0.28 milestone Jun 23, 2022
@guimachiavelli guimachiavelli linked an issue Jun 23, 2022 that may be closed by this pull request
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.

This seems like a solid start to me. The implementation sections should probably be broken up a bit. Curious about why infinite scroll wasn't offered as an option. Too much labor to create code samples for it?

learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Show resolved Hide resolved
@guimachiavelli guimachiavelli changed the base branch from v0.28 to v0.28-pagination June 28, 2022 10:54
guimachiavelli and others added 4 commits June 28, 2022 16:47
Co-authored-by: Tommy <68053732+dichotommy@users.noreply.github.com>
Co-authored-by: Tommy <68053732+dichotommy@users.noreply.github.com>
Co-authored-by: Tommy <68053732+dichotommy@users.noreply.github.com>
@guimachiavelli
Copy link
Member Author

guimachiavelli commented Jun 29, 2022

@bidoubiwa and @gmourier: This is the first draft of the pagination guide. As we discussed, I adapted your original article into something a bit more focussed on explaining why page selectors are not a good fit for Meilisearch and directing users to a recommended solution.

Instructions for reviewers

As usual, all feedback is welcome, but if you're on a rush, what I need the most from you is:

  • Is all the information there correct? Code samples very much included;
  • Do you think we need a code sample for the limit workaround (L#112)?
  • @dichotommy has started a couple of discussions regarding the workarounds (one asking about using maxTotalHits instead of limit and another regarding whether we should suggest users create page selection UIs with estimatedTotalHits when the experience can be subpar). Any thoughts?
  • Is there any critical info missing? I had to cut a lot of relevant content to keep the guide relatively concise. It's possible I went too far.

Other pagination methods

@dichotommy has suggested adding the other pagination methods (e.g. infinite scrolling). While I'm not opposed to it, I am concerned that this guide might already be too long. Perhaps we could make smaller guides with instructions on how to implement other UIs in the future?

CI errors

You can safely ignore those, as they're related to links to pages created in another PR.

@guimachiavelli guimachiavelli marked this pull request as ready for review June 29, 2022 10:59
@guimachiavelli guimachiavelli mentioned this pull request Jun 30, 2022
17 tasks
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.

Some remarks to consider for SEO and internal knowledge and some very light rephrasing suggestions. You do what you want with it Gui, you are the master on board.

Thank you very much for this guide. 🏆

Is all the information there correct? Code samples very much included;

I didn't look at the code samples in detail. @bidoubiwa will have a better eye than me. Other than that, everything seems correct!

Do you think we need a code sample for the limit workaround (L#112)?

I don't think so but again @bidoubiwa will have a better suggestion than me since she experienced a lot of back and forth with struggling users over time around this subject.

@dichotommy has suggested adding the other pagination methods (e.g. infinite scrolling). While I'm not opposed to it, I am concerned that this guide might already be too long. Perhaps we could make smaller guides with instructions on how to implement other UIs in the future?

The main goal as we discussed @guimachiavelli is to indicate to users that a page selection is to be avoided while giving a workaround. It seems to me to be brilliantly done.

We can absolutely add other methods in the future and why not in a much more advanced guide within a resource center.

learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Show resolved Hide resolved
learn/advanced/pagination.md Show resolved Hide resolved
learn/advanced/pagination.md Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com>
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

🔥

learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Show resolved Hide resolved
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.

Just pointing out that the images are still in the file changes.

Thank you for this guide @guimachiavelli and thanks to you @bidoubiwa for the code snippets. Well done!

learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
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 a few changes

  1. I think you forgot to add '/learn/advanced/pagination', to config.js
  2. Should we use "Previous" and "Next" buttons instead of previous and next buttons

learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
learn/advanced/pagination.md Outdated Show resolved Hide resolved
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

This guide is exactly what I expected to help the users do the migration btw v0.27.0 and v0.28.0, following the nbHits change! Thank you very much! Will be definitely helpful!

@curquiza
Copy link
Member

curquiza commented Jul 6, 2022

Will this guide's link be https://docs.meilisearch.com/learn/advanced/pagination.html?

guimachiavelli and others added 3 commits July 6, 2022 15:42
Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com>
Co-authored-by: Maryam <90181761+maryamsulemani97@users.noreply.github.com>
@guimachiavelli
Copy link
Member Author

@maryamsulemani97: thanks for both suggestions 👍

@curquiza: yes, the final link should be https://docs.meilisearch.com/learn/advanced/pagination.html

Also, the good stuff here can be directly attributed to @gmourier and @bidoubiwa ❤️. I only cleaned up things a bit and made the text shorter to make it less intimidating for users.

@guimachiavelli
Copy link
Member Author

bors merge

bors bot added a commit that referenced this pull request Jul 7, 2022
1753: v0.28: Pagination guide r=guimachiavelli a=guimachiavelli

Closes #561

Co-authored-by: gui machiavelli <hey@guimachiavelli.com>
Co-authored-by: gui machiavelli <gui@meilisearch.com>
@bors
Copy link
Contributor

bors bot commented Jul 7, 2022

Build failed:

@guimachiavelli guimachiavelli merged commit b1861b3 into v0.28-pagination Jul 7, 2022
@guimachiavelli guimachiavelli deleted the v0.28-pagination-guide branch July 7, 2022 10:23
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.

Guide on pagination
6 participants