Bookmark sorting UI follow-up

[KyleJu]

A follow-up for #952 (review). The changes include:

• Disable sorting on the headers when it is already sorted based on bookmark queries
• Indicate the query sorting from the name header, with an arrow icon and a description
• Add override_num_param to override the num parameter. It helps accommodate the front-end sorting in bookmark queries, which should be <25

[jcscottiii]

I tried this PR out and I saw a timing bug.

How to replicate:

1. Locally change apiUrl in frontend/src/static/index.html to "https://api.webstatus.dev" (as mentioned here)
2. Open your browser to http://localhost:5555/
3. Let the page load up completely
4. Click the ordered bookmark Top CSS Interop Issues.
5. I see the Unable to apply custom sorting to bookmark "Top CSS Interop issues". Defaulting to normal sorting. message come up quickly. But I see the right results eventually.

I suspect that we attempt custom sorting on the old results loaded in step 2. Custom sorting fails because that list of results does not have the list of results. But then he results from step 4 finally load in and show the right results.

[KyleJu]

I tried this PR out and I saw a timing bug.

How to replicate:

1. Locally change apiUrl in frontend/src/static/index.html to "https://api.webstatus.dev" (as mentioned here)
2. Open your browser to http://localhost:5555/
3. Let the page load up completely
4. Click the ordered bookmark Top CSS Interop Issues.
5. I see the Unable to apply custom sorting to bookmark "Top CSS Interop issues". Defaulting to normal sorting. message come up quickly. But I see the right results eventually.

I suspect that we attempt custom sorting on the old results loaded in step 2. Custom sorting fails because that list of results does not have the list of results. But then he results from step 4 finally load in and show the right results.

Spent some time digging into it after chatting offline. I think the root of the issue is caused by this.taskTracker not resetting properly when reload a new query. It is supposed to happen in webstatus-overview-page.ts.

I narrowed down the problem to this line, this.loadingTask = new Task(. It does some funky stuff and reassigns an old value to this.taskTracker before this._fetchFeatures(apiClient, routerLocation); callback is complete. You can verify it by setting a breakpoint at the line 64 and 65 and examine the value of this.taskTracker . I will look more into it tomorrow.

[jcscottiii]

@KyleJu @jrobbins I feel like the disabling of the sort here is similar to https://github.com/GoogleChrome/webstatus.dev/pull/995/files

We should only have one way to disable the sort. This approach allows us to provide a specific message. In 995, we could just toggle it by setting the specific CELL_DEF to unsortable. I wonder if there's a way to join the two.

[KyleJu]

Will address the comments after #995 is merged

[KyleJu]

@KyleJu @jrobbins I feel like the disabling of the sort here is similar to https://github.com/GoogleChrome/webstatus.dev/pull/995/files

We should only have one way to disable the sort. This approach allows us to provide a specific message. In 995, we could just toggle it by setting the specific CELL_DEF to unsortable. I wonder if there's a way to join the two.

I couldn't find a great way to join two, but any suggestions are welcome!

[jcscottiii]

You could modify renderHeaderCell like this:

export function renderHeaderCell(
  routerLocation: {search: string},
  column: ColumnKey,
  sortSpec: string,
+  overrideColumnOpitons?: Map<
+    ColumnKey,
+    {title?: string; unsortable?: boolean}
+  >,
): TemplateResult {

Then back in render in webstatus-overview-table, you could do something like this:

+    let options:
+      | Map<ColumnKey, {title?: string; unsortable?: boolean}>
+      | undefined = undefined;

    if (this.bookmark !== undefined && this.bookmark?.is_ordered) {
      const bookmarkName = this.bookmark.name;
+      options = new Map<ColumnKey, {title?: string; unsortable?: boolean}>();
+      Object.values(ColumnKey).forEach(column => {
+        const value: {title?: string; unsortable?: boolean} = {
+          unsortable: true,
+        };
+        if (column == ColumnKey.Name) value.title = bookmarkName;
+        options?.set(column, value);
+      });
    }
...
...
...
    return html`
      <table class="data-table">
        ${renderColgroups(columns)}
        <thead>
          ${renderGroupsRow(columns)}
          <tr class="header-row">
            ${columns.map(
              col =>
-                html`${renderHeaderCell(this.location, col, sortSpec)}`,
+                html`${renderHeaderCell(this.location, col, sortSpec, options)}`,
            )}
          </tr>
        </thead>
        <tbody>
          ${this.renderTableBody(columns)}
        </tbody>
      </table>
    `;

You will need to modify renderHeaderCell to use the options if present. Maybe something like this: if (colDef.unsortable || overrideColumnOpitons?.get(column)?.unsortable).

[jcscottiii]

LGTM with one nit. Talked offline with Kyle. He said he will adjust the tests.