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

fix: allow up to 100 namespaces and sort them #2606

Merged
merged 5 commits into from Jun 22, 2023
Merged

Conversation

ciyer
Copy link
Contributor

@ciyer ciyer commented Jun 19, 2023

Some users have more than 20 namespaces. Increase the limit used when showing the namespace list in the project creation dropdown.

/deploy #persist #cypress

@ciyer ciyer temporarily deployed to renku-ci-ui-2606 June 19, 2023 09:37 — with GitHub Actions Inactive
@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-ui-2606.dev.renku.ch

@ciyer ciyer temporarily deployed to renku-ci-ui-2606 June 20, 2023 08:15 — with GitHub Actions Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-2606 June 21, 2023 06:34 — with GitHub Actions Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-2606 June 21, 2023 08:39 — with GitHub Actions Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-2606 June 21, 2023 10:27 — with GitHub Actions Inactive
@ciyer ciyer marked this pull request as ready for review June 21, 2023 12:04
@ciyer ciyer requested a review from a team as a code owner June 21, 2023 12:04
@ciyer ciyer temporarily deployed to renku-ci-ui-2606 June 21, 2023 14:30 — with GitHub Actions Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-2606 June 21, 2023 16:40 — with GitHub Actions Inactive
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

This works as advertised. Since it's intended as a quick fix, we can merge it as is.

Ideally, it would be nice to move the Namespace type to a definition file and parametrize both per_page and owned_only since we are touching that code already.

@ciyer ciyer temporarily deployed to renku-ci-ui-2606 June 22, 2023 06:51 — with GitHub Actions Inactive
@rokroskar
Copy link
Member

why not use pagination properly? This is just going to break for someone else later.

@ciyer
Copy link
Contributor Author

ciyer commented Jun 22, 2023

why not use pagination properly? This is just going to break for someone else later.

Because this allows us to get a quick fix for the immediate problem, but longer-term, then UX for this component really needs to be rethought.

Currently, we assume that users have a small number of namespaces they can access, and we show them a full list of their namespaces.

image

If the user has access to so many namespaces that pagination is necessary, this component will be unusable anyway. Instead, we need to do something like show a list of likely namespaces the user might want to pick (for example, recently accessed ones), but let them type input, search against that and show them a list of matches, which they can pick from.

@ciyer ciyer deployed to renku-ci-ui-2606 June 22, 2023 12:50 — with GitHub Actions Active
@lorenzo-cavazzi
Copy link
Member

lorenzo-cavazzi commented Jun 22, 2023

I suggest merging this as is since it's just a quick fix and users have asked for it.
As a side note, text from the user is already used for filtering the namespaces.

We can definitely improve the logic but I would like to explore a more comprehensive solution for pagination that we can reuse for similar cases. The React Toolkit library we are using doesn't provide an out-of-the-box solution but suggests a way (REF).

In general, the solution should make it easy to assess at any point whether some items have already been fetched and whether more are loading. In this case, after fetching the first page, we can let the user pick a namespace already and show a loading wheel to let the user know we are still loading more items.
I'll make a follow-up issue.

@ciyer ciyer merged commit 203db6f into master Jun 22, 2023
14 checks passed
@ciyer ciyer deleted the ciyer/000-namespace-fix branch June 22, 2023 14:09
@RenkuBot
Copy link
Contributor

Tearing down the temporary RenkuLab deplyoment for this PR.

@lorenzo-cavazzi
Copy link
Member

Follow-up issue: #2618
Comments are ofc welcome 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants