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

Expose combined search/list pages for packages and dependencies #458

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

rkent
Copy link

@rkent rkent commented Jan 2, 2025

This PR exposes the new combined list and search pages in the main UI rosindex.

A significant change here is that the home page and the package list&search page have been combined into a single page. The reason for this is that I wanted to optimize what I believe is a common use case: Go to rosindex, and search for a particular package. There are two large files that need loading for this. The data file is needed to display a package list. This index file is needed for search. As with the prior /search_packages page, the data file loading starts in the background when entering the page, and the index file loading starts when the search field gets focus to enter a search term. The expectation is that by the time the user starts the search, all data is loaded and the search is very fast. If we do not combine these pages, and instead load /search_packages for the home page, then the data and index download would not start until that page was loaded, with a potential significant delay.

Because exposing the new combined list&search pages is a significant change to the way that rosindex functions, it was important to incorporate a variety of changes to keep rosindex functioning appropriately with this change. That makes this a rather long PR, and one that includes a number of changes that could in principle be done in a series of smaller PRs, but in practice would be difficult to split up. Here is a list of the changes:

  • combine search_packages and home/index into single page, including removing the /search_packages page.
  • common look and feel across all pages.
  • add help to "About".
  • Add a help page for package search.
  • Remove extraneous "-" from default layout.
  • Move "Enable search" box to behind the search text.
  • Revise tutorial to support new search. Move tutorial to Help section.

A full rosindex run with this PR (and other pending PRs) loaded is available at https://pr-rosindex.rosdabbler.com

@tfoote
Copy link
Member

tfoote commented Jan 8, 2025

Sorry I missed the notifications on your other PRs they're merged now.

A few bits of feedback.

Testing the search page all the results point to a specific instance of the package instead of the canonical url aka

https://pr-rosindex.rosdabbler.com/p/action_msgs/github-ros2-rcl_interfaces/#rolling
vs
https://pr-rosindex.rosdabbler.com/p/action_msgs/#rolling

It would be great to prefer the shorter form. It's on my list to remove support for the longer form with different instances. It's now basically duplicate content with a longer url which is bad for useability as well as public indexing.


It's great bringing the listings to the home page. I think most of the legacy content can be cleared out and added to the header.

For example add the following drop down next to ROS Resources, removing the "Documentation" sublink

ROS Distribution Documentation

  • Noetic
  • Humble
  • Jazzy
  • Rolling

The Resources in the home page are already in the ROS Resources header.

That only leaves the top blurb welcome message which maybe we could make into a floating top item that disappears if you type in a specific search or if you scroll down it would just scroll off the top of the page.

Such that when people first hit the page they'll see the long list and it will be pretty obvious what they can find on the page.

Kind of like this results but with the introduction/welcome message smaller and dismissable or scrolls away.

image

Note that this isn't photoshop but an interesting bug/side effect if you press "back" after searching for something.

It could also be that we can fit this content beside the search bar, especially if in the future the search gets a few more options and filtering and might take up more than just a single line.

@tfoote tfoote self-requested a review January 8, 2025 01:42
@rkent
Copy link
Author

rkent commented Jan 8, 2025

I agree that the home page content is duplicative and not very interesting, but couldn't that be done in a followup PR? I'm trying to focus on just getting the new package stuff exposed here. What is there is what was there before.

If you have specific reproducible bad behavior with the history buttons, please report it. There is code that tries to make the history work reasonably, so if there are remaining issues I'd like to fix it.

I have no problem linking to the more generic package page, the existing link just duplicated what was there before.

Something I've discovered that I want to fix is that the breadcrumbs for packages and deps point back to the old list page. I want to fix that so they point to the new list pages. There should not be any links to the old pages in the UI (even though I have not removed those yet).

There is also an odd behavior I've seen but not tracked down. Sometimes after a search, I get a blank page until I scroll. I need to reproduce this and fix it.

Because this PR is pretty major, it blocks most additional PRs. It would be good to be explicit about what needs to be fixed before it can be landed.

There are problems trying to combine additional variable-height content before the Tabulator table, such as your proposed home page. If you remember, in the initial iterations of this we ended up with two scroll bars. I've tried hard to eliminate that, but it requires a fixed-height page that fills the view screen but no more. I could add material at the top and reduce the height of the Tabulator table for the "home" page, or just live with two scroll bars, but those are compromises that might be more problematic in mobile views. I'm not convinced yet that showing the Tabulator table on the home page is worth the compromises. Also note that an initial display of the table would cause a delay as the data has to be loaded first. The main goal for combining the home page with the table was hiding that delay.

@rkent
Copy link
Author

rkent commented Jan 8, 2025

I'll do another commit that fixes the minor issues, then we can discuss if you want any larger proposed changes.

@tfoote
Copy link
Member

tfoote commented Jan 10, 2025

You're right lets do the home page re-layout in a follow up.

Here's the bug with the current rendering.
Screencast from 2025-01-09 18-18-09.webm

To reproduce:

  • go to the home page
  • search for something
  • press the back button
  • -> The old search results stay visible below the static content.

@tfoote
Copy link
Member

tfoote commented Jan 14, 2025

Overall it's looking good. Though I think that I found a bug. If you do a search and then click to switch to another distro it uses 100% CPU for >5 seconds on my machine before rerendering the table. The console output implies that the table filtering has completed promptly, but there's a long delay before rendering.

Screencast.from.2025-01-14.10-53-12.mp4

@rkent
Copy link
Author

rkent commented Jan 15, 2025

I can reproduce the behavior you are seeing, and I will investigate.

@rkent
Copy link
Author

rkent commented Jan 20, 2025

I think I understand what happened here, and just pushed a fix. But don't review yet, I want to do a full build with this first.

Explanation: by adding class showAsSearch to the div with class tab-table, then the style was being overwritten when we switched from Home page to Search page. But that style also had the height of the tabulator table. Because it was overwritten, the height was not constrained. I don't fully understand why that error was not visible, but somehow the effect within Tabulator was that is really slowed down the rendering, as now we are rendering all >1000 rows as displayed instead of hidden. Switching to jquery css instead of attr should solve this.

@tfoote
Copy link
Member

tfoote commented Jan 21, 2025

I tried iterating it seemed to be rerendering the table several times in a row. Let me know when your rebuild is done and I'll take another look.

@rkent rkent force-pushed the 2-expose-new-search branch from efc764e to 3bb12f8 Compare January 21, 2025 21:35
@rkent
Copy link
Author

rkent commented Jan 21, 2025

The rebuild is done. Current https://pr-rosindex.rosdabbler.com should match this PR branch with a couple of trivial exceptions (added missing var declarations on a couple of lines).

I've been traveling, and a little sick, so that has slowed me down the last 10 days.

I think it is working well now. On the issue of double rendering: two issues here.

First, in order to change the filter values on the table if the query changes, I have to first remove the old filter, then add the new. When the "remove" occurs, I get a table filtered event showing all rows, then a second event on adding the new filter. This happens so fast that I cannot tell if the table is re-rendered between these. I don't see a way (short of doing unsupported Tabulator manipulations) to prevent this if I change the filter of an active table.

Second, we are listening both on popstate as well as hashchange. Sometimes changes fire only one of these, but sometimes if fires both. If it fires both, then the table is rebuilt twice. I did some experiments with preventing this second re-render (storing the query and hash globally, and only re-rendering if it changes) that seems to work - but I would want to test it more thoroughly. I could implement that with this PR, or as a followup.

Another known issue is that Jekyll complains that we have double defined the root index.html. I can't see an effect of this, but it needs to be tracked down:

          Conflict: The following destination is shared by multiple files.
                    The written file may end up with unexpected contents.
                    /home/kent/rosindex/_site/index.html
                     - index.html
                     - index.html

Again, I could do this before or after this PR.

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

This provides a much better experience as it is, even with the double render it's much faster than the current search load anyway. We can resolve the double render in a followup.

@tfoote tfoote merged commit 5c2c8de into ros-infrastructure:ros2 Jan 22, 2025
1 check passed
@rkent rkent deleted the 2-expose-new-search branch January 23, 2025 22:11
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