Problem/Motivation

When you change a filter or change the page you're on, the entire existing set goes away (but not the pager or the results count), and then it pops in. This is a bit of a janky experience.

Steps to reproduce

Load the drupal.org contrib source plugin page, then change to page 2 (or change a filter).

Proposed resolution

Instead of immediately disappearing everything, "dim" the existing set and then replace it with the new set, scrolling to the top once loading is complete.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

chrisfromredfin created an issue. See original summary.

omkar-pd made their first commit to this issue’s fork.

omkar-pd’s picture

Status: Active » Needs review
StatusFileSize
new11.87 MB

Raised PR with Proposed resolution.

PB

chrisfromredfin’s picture

Title: UI enhancement » UI enhancement (paging/filtering improvements)
narendrar’s picture

Status: Needs review » Needs work

Tested manually and observed that the page scrolls on initial load (when navigating to /admin/modules/browse/drupalorg_jsonapi, the page automatically scrolls to the list). This behaviour seems incorrect to me.

utkarsh_33 made their first commit to this issue’s fork.

utkarsh_33’s picture

All the tests that are failing are due to the extra wait that we need due to this change.I think this is not a good way.We might need something more robust here.Will try to find a better solution.
Also @omkar-pd if you think you have any better approach inline with what you added it would be great if either you can suggest or push the fix related to what's mentioned in #6.

utkarsh_33’s picture

I have pushed some changes that fixes tests.Most of the remaining tests can also be fixed in same manner, but i wonder if we really want to make changes in tests like this.I'll ask @phenaproxima about whether we want to do this.In the mean time if @omkar-pd has a better solution, then it's perfect.

phenaproxima’s picture

I do like the proposed resolution, although I think we should remove the bit that scrolls you directly to the projects, as @narendrar pointed out in #6. Let's just remove that outright.

The test changes do make me a bit nervous, though -- we are, if I remember correctly, now bypassing all of these waits. I sorta wonder why this causes the tests to pass.

This is frankly why I'd like to remove clickWithWait() and replace it with something better. Not yet sure what that would look like, though.

utkarsh_33’s picture

Status: Needs work » Needs review

The tests are now passing.

phenaproxima’s picture

Title: UI enhancement (paging/filtering improvements) » When paging or filtering is changed, scroll the user back to the top of the project browser
Status: Needs review » Needs work

I dig this change. I think it increases the amount of polish - nice work.

I gave it a quick manual test on a page with one instance of Project Browser, but didn't test it with multiple instances yet.

The code looks reasonably straightforward but feels a little overcomplicated for what it's doing. It also needs more comments, and there appear to be out-of-scope test changes in there (which, to be honest, were probably introduced by me while I was fixing merge conflicts).

phenaproxima’s picture

Thanks for helping me analyze this, @utkarsh_33. 🙏

I'm not entirely certain here. The current number of flags still feels somewhat...imprecise to me, and we need to keep the Svelte code as clean as we can because it is prone to messiness. :)

I'd like to see what happens if we start removing flags.

phenaproxima’s picture

I realized, while reviewing this again, that one of the major reasons the code feels awkward is because it's taking place in ProjectGrid.svelte, which is totally disconnected from the loading code.

Is there any chance we can move this stuff into ProjectBrowser.svelte? It understands when loading is happening, and that, to me, feels like a much more natural place to call scrollIntoView().

Besides, we should probably put the overlay over the entire project browser, including its filters and pagers, so that users can't interact with it at all while loading is going on. That makes more sense to me.

narendrar’s picture

Status: Needs work » Needs review

Changes done as suggested in #14

phenaproxima’s picture

Status: Needs review » Needs work

This seems to work mostly as intended, with one problem: while the loading overlay is present, I can still interact with the Categories dropdown if it's open.

I'm thinking the solution here is to ensure that the overlay sits, well, over the dropdown. z-index bug, maybe?

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Decided to fix the z-index bug myself; wasn't tricky.

I tested this both on /admin/modules/browse routes, and with two project browsers placed on the same page as blocks. They worked as expected.

There was one bug that it is probably pre-existing: if you click a pager item in a second instance of project browser, you get jumped up to the same pager item in the first instance. That is almost certainly pre-existing and calls for a follow-up.

But overall, this is behaving rightly and looking good, and the code makes perfect sense. I think I'm happy with it.

phenaproxima’s picture

omkar-pd’s picture

Tested this and it's working as expected. 🎉

chrisfromredfin’s picture

Status: Reviewed & tested by the community » Fixed

I've wanted this for so long; this is such a superior experience in such a subtle way.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.