FIX: empty state message on the user bookmarks page (PR #14257)

After adding an empty state banner tot the user bookmarks page, we have found the bug. Steps to reproduce:

  1. Go to the user bookmarks page
  2. Search for something that doesn’t exist in bookmarks
  3. Click again Bookmarked on the sidebar or View All Bookmarks on the user menu again

You’ll see the empty state banner:

This is the problem with routing, and it’s hard to fix because of how this page is implemented. We’re not loading the model in the model hook in the route, so when transition happens ember call the model hook, but the loading isn’t happening.

This PR remove code that load bookmarks from the ember model. In general, this is an antipattern to be doing that. I moved initial loading to the model hook in the route, and loading of additional bookmarks that is triggered when a user scroll down the page - to the controller. This fixes the bug, and it also will save us from problems in the future when touching this code or migrating to new Ember versions.

GitHub

@martin-brennan Seems like the best person to review this :grin:

Looks like a way better way to do this, thanks for making the change, just got a couple of comments.

I don’t understand why this get/set pattern is needed here on the computed property?

We are trying to move away from jQuery stuff. I think the easiest way would be to do the same as:

Ideally we should have this kind of thing in a library function to compensate for the loss of jQuery.

I’ve got rid of this jQuery call. I agree that we’re probably going to need a library function to compensate the loss of JQuery. But I don’t need it for this change because I’ve simplified the signature of the function. We don’t need to pass additionalParams object to the function, since the only parameter we actually pass is searchQuery, now it’s simpler and more explicit:

  _loadMoreBookmarks(searchQuery) {
    if (!this.model.loadMoreUrl) {
      return Promise.resolve();
    }

    let moreUrl = this.model.loadMoreUrl;
    if (searchQuery) {
      const delimiter = moreUrl.includes("?") ? "&" : "?";
      const q = encodeURIComponent(searchQuery);
      moreUrl += `${delimiter}q=${q}`;
    }

    return ajax({ url: moreUrl });
  },

We need this one-way binding: qsearchTerm.

q is a search string from HTTP parameters, searchTerm is the property that’s bound to the search box on the page. We need the one-way binding because we don’t want q to be changed when a user is typing a search term. It would cause many requests to the server, we need to debounce them then. Also, it’s a bit strange to see that the URL in the address bar is changing when you’re typing something into the search box. We set q to the value of searchTerm only in the end, when a user presses Enter.

But when we do this update on Enter, one way binding stops working:

Because of this, sometimes after reentering the route with a new value of q, the search box on the page was still containing the previous search term. Defining setter resolves the problem. I would think of this pattern as about one-way binding with the ability to set the value when you need to (the value you’re setting will persist until the property is recalculated).

Note, that you’ll see this warning only if you implement a one-way binding like this:

@computed("q")
searchTerm(q) {
   return q;
}),

If you use the oneWay macros behavior will be the same, but you won’t see the warning.

I initially was trying to implement it using the oneWay macros, had some hard time trying to figure out what’s wrong :).

Looks great, and yeah I wasn’t implying we needed that replacement lib done in this PR, I just meant in general it would be good if we had one :slight_smile:

Thanks for the thorough explanation, that makes sense!