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

Sort displayed features based on bookmark query #952

Merged
merged 4 commits into from
Dec 10, 2024
Merged

Sort displayed features based on bookmark query #952

merged 4 commits into from
Dec 10, 2024

Conversation

KyleJu
Copy link
Collaborator

@KyleJu KyleJu commented Dec 4, 2024

A follow-up for #764, sort the features in the overview page based on the bookmark query order.

  • Given that the number of features should be < 20, the sorting is implemented on the frontend
  • Again given the number, the algorithm for finding a feature is brute force
  • Only support id and name at the moment
  • If there are features missing after sorting, revert to the default order

@KyleJu KyleJu force-pushed the sort-bookmark branch 2 times, most recently from f452692 to bd55fe5 Compare December 5, 2024 02:18
@KyleJu KyleJu changed the title [draft] Finish implementation Sort displayed features based on bookmark query Dec 5, 2024
@KyleJu KyleJu requested a review from jcscottiii December 5, 2024 02:28
@KyleJu KyleJu marked this pull request as ready for review December 5, 2024 02:29
Copy link
Collaborator

@jcscottiii jcscottiii left a comment

Choose a reason for hiding this comment

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

Thanks for this PR and I like the unit tests file you added.

One thing to note: You should rebase your code because you will need to set is_ordered on the newest query. Otherwise, CI for this PR will fail in the merge queue.

We need to handle this edge case:

That will render 10 items even though we should render 12. But the sorting will be incorrect because the backend only sends the first 10 according to its sorting logic.

We should have a way to reset the number of values loaded from the backend to a number that makes sense.

Easiest way to do that would be handle it during the rendering of the bookmark url

renderBookmark(bookmark: Bookmark, index: number): TemplateResult {
const bookmarkId = `bookmark${index}`;
const currentLocation = this.getLocation();
const currentURL = new URL(currentLocation.href);
const bookmarkUrl = formatOverviewPageUrl(currentURL, {
q: bookmark.query,
start: 0,
});

Additionally, when someone clicks on the CSS bookmark, we still indicate that the baseline column is being sorted. But that is not the case when using this custom sort.

We should also add a test that asserts that the actual bookmarks with is_ordered = true in DEFAULT_BOOKMARKS can work. The test queries in frontend/src/static/js/components/test/webstatus-overview-table.test.ts are good for getting the basic and edge cases but we should make sure that our actual CSS bookmark will work as expected. This can come as another unit test.

@KyleJu
Copy link
Collaborator Author

KyleJu commented Dec 10, 2024

Thanks for this PR and I like the unit tests file you added.

One thing to note: You should rebase your code because you will need to set is_ordered on the newest query. Otherwise, CI for this PR will fail in the merge queue.

We need to handle this edge case:

That will render 10 items even though we should render 12. But the sorting will be incorrect because the backend only sends the first 10 according to its sorting logic.

We should have a way to reset the number of values loaded from the backend to a number that makes sense.

Easiest way to do that would be handle it during the rendering of the bookmark url

renderBookmark(bookmark: Bookmark, index: number): TemplateResult {
const bookmarkId = `bookmark${index}`;
const currentLocation = this.getLocation();
const currentURL = new URL(currentLocation.href);
const bookmarkUrl = formatOverviewPageUrl(currentURL, {
q: bookmark.query,
start: 0,
});

Additionally, when someone clicks on the CSS bookmark, we still indicate that the baseline column is being sorted. But that is not the case when using this custom sort.

We should also add a test that asserts that the actual bookmarks with is_ordered = true in DEFAULT_BOOKMARKS can work. The test queries in frontend/src/static/js/components/test/webstatus-overview-table.test.ts are good for getting the basic and edge cases but we should make sure that our actual CSS bookmark will work as expected. This can come as another unit test.

There are three things to address in this comment:

  1. An edge case when the num param in present
  2. UI indicator for sorting
  3. A test case for real bookmark value.

Let me address the third item in this PR and send out a follow-up PR for the first two items, for the ease of review.

@KyleJu
Copy link
Collaborator Author

KyleJu commented Dec 10, 2024

Note to myself: rebase my code before merge

@KyleJu KyleJu requested a review from jcscottiii December 10, 2024 00:38
Copy link
Collaborator

@jcscottiii jcscottiii left a comment

Choose a reason for hiding this comment

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

LGTM.

An addition to one of the future PRs: Move bookmark out of constants.ts and into its own bookmark.ts file in utils. With that, you either:

  1. Move findFeaturesFromAtom and reorderByQueryTerms there as exported functions and leave Bookmark as an interface that also lives in the same bookmark.ts file.
  2. Turn the bookmark interface into class and add findFeaturesFromAtom (private) and reorderByQueryTerms (exported) as methods in that class.

In either case, you will want to throw an error when the length does not match and then the code here will do the toast show.

This will keep this component strictly about rendering and any complex logic will be pulled out.

@KyleJu KyleJu enabled auto-merge December 10, 2024 17:56
@KyleJu KyleJu added this pull request to the merge queue Dec 10, 2024
Merged via the queue into main with commit c520f24 Dec 10, 2024
6 checks passed
@KyleJu KyleJu deleted the sort-bookmark branch December 10, 2024 21:31
@KyleJu
Copy link
Collaborator Author

KyleJu commented Dec 20, 2024

  1. Move findFeaturesFromAtom and reorderByQueryTerms there as exported functions and leave Bookmark as an interface that also lives in the same bookmark.ts file.

Filed #1001 for this follow-up

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.

2 participants