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

Make Browse models table sortable #42113

Merged
merged 3 commits into from May 9, 2024
Merged

Make Browse models table sortable #42113

merged 3 commits into from May 9, 2024

Conversation

rafpaf
Copy link
Contributor

@rafpaf rafpaf commented May 1, 2024 •
edited

This PR adds sorting functionality to the models table. You can sort by a model's name or by the path of collections. Descriptions are not sortable.

Kapture 2024-05-08 at 14 54 01

This was referenced May 1, 2024
Closed
Merged
Merged
@rafpaf rafpaf mentioned this pull request May 1, 2024
Merged
@metabase-bot metabase-bot bot added the .Team/AdminWebapp Admin and Webapp team label May 1, 2024
This was referenced May 1, 2024
Merged
Merged
@rafpaf Graphite App
Copy link
Contributor Author

rafpaf commented May 1, 2024 •
edited

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rafpaf and the rest of your teammates on Graphite Graphite

This was referenced May 1, 2024
Closed
Merged
@rafpaf rafpaf requested review from noahmoss and removed request for noahmoss May 1, 2024 18:27
@rafpaf rafpaf marked this pull request as ready for review May 1, 2024 18:27
@rafpaf rafpaf changed the title wip: supporting sorting in Browse models table Make Browse models table sortable May 2, 2024
@rafpaf rafpaf requested a review from camsaul as a code owner May 2, 2024 11:07
@rafpaf rafpaf mentioned this pull request May 2, 2024
Open
@rafpaf rafpaf force-pushed the browse/sort branch 3 times, most recently from 7d946e3 to d7feef3 Compare May 8, 2024 17:44
@rafpaf rafpaf mentioned this pull request May 8, 2024
Merged
@rafpaf rafpaf removed the request for review from camsaul May 8, 2024 18:02
@rafpaf rafpaf force-pushed the browse/sort branch 2 times, most recently from 6338f2b to 33e10e1 Compare May 8, 2024 18:14
@rafpaf rafpaf added the no-backport Do not backport this PR to any branch label May 8, 2024
@rafpaf rafpaf added this to the 0.50 milestone May 8, 2024
iethree
iethree requested changes May 8, 2024
View reviewed changes
Copy link
Contributor

@iethree iethree left a comment

Choose a reason for hiding this comment

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

A few thoughts along the way, but I think the biggest issue is that we should be containing the sorting state within a table component, not persisting sorted state in the BrowseModels container.

frontend/src/metabase/browse/components/BrowseModels.tsx Outdated Show resolved Hide resolved
frontend/src/metabase/browse/components/ModelsTable.tsx Outdated Show resolved Hide resolved
frontend/src/metabase/browse/components/utils.tsx Show resolved Hide resolved
frontend/src/metabase/browse/components/utils.unit.spec.tsx Outdated Show resolved Hide resolved
frontend/src/metabase-types/api/search.ts Outdated Show resolved Hide resolved
@iethree
Copy link
Contributor

iethree commented May 8, 2024

PS, sorting by name on 1000 models takes almost 11 seconds 😞

Screen Shot 2024-05-08 at 3 03 58 PM

even for 200 models it takes almost a second and feels slow

Screen Shot 2024-05-08 at 3 08 50 PM

Base automatically changed from browse/nav to master May 8, 2024 22:08
@rafpaf rafpaf mentioned this pull request May 9, 2024
Closed
@rafpaf rafpaf requested a review from iethree May 9, 2024 11:05
iethree
iethree approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@iethree iethree left a comment

Choose a reason for hiding this comment

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

🥳

@rafpaf rafpaf merged commit e9b4378 into master May 9, 2024
112 checks passed
@rafpaf rafpaf deleted the browse/sort branch May 9, 2024 15:04
@rafpaf Graphite App
Copy link
Contributor Author

rafpaf commented May 9, 2024

Merge activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iethree iethree iethree approved these changes

Assignees

@rafpaf rafpaf

Labels
no-backport Do not backport this PR to any branch .Team/AdminWebapp Admin and Webapp team
Projects
None yet
Milestone
0.50
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rafpaf @iethree