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

Tools page customization #508

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

miken32
Copy link

@miken32 miken32 commented Dec 22, 2023

Implement feature request for customization of lists at https://stackoverflow.com/tools as described in #505.

One thing lacking is inability to add radio buttons to the settings to select question/answer/all. Like you can add a single setting of type "radio" but without ability to link them together it's kind of useless.

Also was unable to get sox.helpers.addAjaxListener to work, but didn't spend much time figuring out why since I just went with the mutation observer instead.

Copy link
Member

@shu8 shu8 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR, I'm really sorry for the delay in looking at it!

I've just got some minor comments, and a small syntax error, but otherwise this LGTM!

sox.features.js Show resolved Hide resolved
sox.features.info.json Outdated Show resolved Hide resolved
sox.features.info.json Outdated Show resolved Hide resolved
sox.features.js Outdated
rows.forEach((row, i) => row.classList.toggle("collapsing", i >= listCount));
};
// tables are populated by XHR after page loads
sox.helpers.observe(Array.from(document.querySelectorAll("div.island")), "table.summary-table", trimLists);
Copy link
Member

Choose a reason for hiding this comment

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

We could replace this with sox.helpers.addAjaxListener like this:

      // tables are populated by XHR after page loads
      sox.helpers.addAjaxListener("\\/tools\\?tab=", () => {
        document.querySelectorAll("table.summary-table").forEach(trimLists);
      });
      // just in case we come in after a table has been loaded already
      document.querySelectorAll("table.summary-table").forEach(trimLists);

we found that the advantage of doing it this way is we stop adding so many mutation observers to the page, which seemed to have negative performance effects when there were too many.

This method instead just triggers your function whenever an xhr response from the given URL is received, after which we can query for the table once

@miken32
Copy link
Author

miken32 commented Apr 29, 2024

I'm going to convert this to draft for now and do some more testing since it's been a while and seems to be acting a bit flaky for me. I'm still not able to get the ajax listener working, FWIW.

@miken32 miken32 marked this pull request as draft April 29, 2024 19:46
@shu8
Copy link
Member

shu8 commented Apr 30, 2024

Hm, it seemed to work for me when I tried it locally, does nothing happen when you use the ajax listener?

@miken32
Copy link
Author

miken32 commented May 3, 2024

Hm, it seemed to work for me when I tried it locally, does nothing happen when you use the ajax listener?

Well, nothing happens at first. Opening up one of the lists triggers the listener, as does changing tabs. Seems inconsistent, and oddly enough only seems to happen when I have the dev console open.

Check this video where you can see I have multiple items shown (the custom setting is at 8 items) then I reload the page and only have 3. You can also see the expected Ajax requests were made. I change tabs and come back, and the listener has fired. It's not a time delay; I can wait 5 minutes and nothing will happen until the tab loses focus and regains it again. Very strange.

@shu8
Copy link
Member

shu8 commented May 18, 2024

I wonder if what's happening is that the tab change triggers the focus, and only then does the feature actually run, due to this code which is what actually triggers Feature functions:

sox/sox.user.js

Lines 181 to 187 in 78d65e7

if (sox.settings.available) {
if (document.hasFocus && document.hasFocus()) {
runFeatures(settings, featureInfo);
} else {
window.addEventListener('focus', () => runFeatures(settings, featureInfo), { once: true });
}
}

But - if this is the case, it's strange that you wouldn't experience the same issue with the Mutation Observer approach.


This is what it looks like for me, with listCount=2, with a tab switch in between:

tools-page

when using this code for the entire feature:

  customizeToolsPageLists: function(settings) {
    const trimLists = function(table) {
      if (settings.filterInvalid) {
        table.querySelectorAll("td.tagged-ignored").forEach(cell => cell.parentNode.remove());
      }
      if (settings.filterTypeQuestions) {
        table.querySelectorAll("a.question-hyperlink").forEach(cell => cell.parentNode.parentNode.remove());
      }
      if (settings.filterTypeAnswers) {
        table.querySelectorAll("a.answer-hyperlink").forEach(cell => cell.parentNode.parentNode.remove());
      }
      const listCount = settings.listCount || 3;
      const rows = table.querySelectorAll("tr");
      rows.forEach((row, i) => row.classList.toggle("collapsing", i >= listCount));
    };
   // tables are populated by XHR after page loads
    sox.helpers.addAjaxListener("\\/tools\\?tab=", () => {
      console.log('fire')
      document.querySelectorAll("table.summary-table").forEach(trimLists);
    });
    // just in case we come in after a table has been loaded already
    document.querySelectorAll("table.summary-table").forEach(trimLists);
  }

In any case, if this definitely doesn't work for you, I'm happy to merge in the Mutation Observer approach, because there's not many other features that run on this page anyway (the main issue with observers used to be when lots of features applied their own to the same page e.g., a post page which lots of SOX features target).

@shu8
Copy link
Member

shu8 commented Aug 28, 2024

Hey @miken32, just wondering if you'd had time to re-test this? It would be great to get this merged in if you're still interested!

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