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

Filebrowser paging draft #539

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cnydw
Copy link

@cnydw cnydw commented Jun 8, 2021

Addressing issue #538

Added a page parameter to the request, to select the pages.
The updated API is not mature , would love to hear suggestions.

@welcome
Copy link

welcome bot commented Jun 8, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@blink1073 blink1073 marked this pull request as draft July 2, 2021 11:18
@@ -106,9 +106,13 @@ async def get(self, path=''):
if content not in {'0', '1'}:
raise web.HTTPError(400, u'Content %r is invalid' % content)
content = int(content)

page = self.get_query_argument('page', default='1')
Copy link
Member

Choose a reason for hiding this comment

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

It would probably be more backwards compatible if it defaulted to not being paginated if the page argument is not given.


if len_contents > MAX_NUM_CONTENTS_PAGE:
# round up the max number of pages
max_page = int(len_contents / MAX_NUM_CONTENTS_PAGE) + (len_contents % MAX_NUM_CONTENTS_PAGE > 0)
Copy link
Member

@mwakaba2 mwakaba2 Jul 17, 2021

Choose a reason for hiding this comment

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

there'a handy function for calculating both div and mod.

pages, contents_remainder = divmod(len_contents, MAX_NUM_CONTENTS_PAGE)
max_page = pages + contents_remainder

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the divmod function! The value of max_page should be the same as math.ceil(len_contents / MAX_NUM_CONTENTS_PAGE), so max_page = pages + contents_remainder isn't correct actually.

# round up the max number of pages
max_page = int(len_contents / MAX_NUM_CONTENTS_PAGE) + (len_contents % MAX_NUM_CONTENTS_PAGE > 0)
model["max_page"] = max_page
if page and (1 <= page) and (page <= max_page) :
Copy link
Member

Choose a reason for hiding this comment

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

There's also a way to combine the comparison operators.
It's also good to explicitly check if page is None because page = 0 can silently return False in this case.

if page is not None and 1 <= page <= max_page:

@mwakaba2
Copy link
Member

mwakaba2 commented Aug 6, 2021

Hi @cnydw, any updates on this? It'd be great to make the filebrowser experience more performant.

@cnydw
Copy link
Author

cnydw commented Aug 6, 2021

@mwakaba2
@vidartf

I made a simple update, however I feel my paging design is far from ideal. The paging design like #538 (comment) isn't very intuitive. Most file browsers don't have pages, instead they only utilize the scrollbar.

A better design would probably abandon the page selection, and will look just like the original file browser. When opening a folder, It will display a small number of files, and when the scrollbar moves to the end, it activates loading more items (if there is any). However this scrollbar activated design will probably not work with my current simple implementation in the jupyter_server backend.

More discussion with JupyterLab side is probably needed.

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

Successfully merging this pull request may close these issues.

None yet

4 participants