FEATURE: Quick access panels in user menu (PR #8073)

CONTEXT: Quick access to bookmarks and messages on user menu

This feature adds quick access panels for bookmarks and personal message. It allows uses to browse recent items directly in the user menu, without being redirected to the full page.

4474a28 shows how the new QuickAccessPanel is extracted from the old UserNotifications, and I hope this will help with the review a bit.

Thanks and open to suggestions!

(I will send another PR to discourse-assign by the end of this week to add its quick access panel.)

GitHub

3 Likes

You’ve signed the CLA, xrav3nz. Thank you! This pull request is ready for review.

The user_actions endpoint does not support a limit parameter, so this truncates the results to ensure they will fit on the screen. I am not sure if it is preferred to add a new limit param to the endpoint?

Same here.

Rather than a showAll action, why not use the url attribute?

Nice functionality! Thanks for making this.

I have a few notes inside.

My feeling is this data would be better stored in the state of a widget, since its value is populated based on the result of the ajax call. Putting the variable here in the closure makes it a bit leaky in tests. Same for emptyPlaceholderItemText below.

Does this need to be an Ember.Object? They are heavier than regular JS objects, and if you don’t need to observe or use computed properties on them I recommend the plain JS version.

To denote a private method we prefix with the underscore, like _findStatleItemsInStore()

It’s a good habit to always return the promise in case something eventually wants to wait on it.

I appreciate the honesty :rofl:

Minor but I’d make these then/catch one liners.

This is probably not a perf issue here, but in general you don’t want to change the shape of the JS objects. https://mathiasbynens.be/notes/shapes-ics

I would recommend setting the values to null instead of delete.

Why is this necessary? Shouldn’t the topic list adapter automatically be returned by name?

In test environment, store seems to be stubbed by this create-store.js.es6 helper. It expects an exact match when lookup, and would otherwise fallback to adapter:rest.

That was a very informative read, TIL! Thanks!

This is actually a verbatim copy from the previous NotificationItem widget, by … some very evil fish. :joy:

Good point, thanks! This was carried over from the original UserNotification widget. I have changed this to a link with href so that right-click/cmd-click will work too!

Totally! I have moved emptyStatePlaceholderItemText to the widget state instead.

However, for stale items, I am under the impression that the widget instance is destroyed when “unmounted”? So,

  1. Open quick access panel for notifications, and staleItems is saved to state.
  2. Switch to quick access panel for bookmarks. The notifications widget along with its state (i.e. staleItems) is destroyed.
  3. Switch back to quick access for notifications. A new notifications widget is initialized –staleItems is empty and the loading circle is shown while fresh items are being fetched.

After having thought about this a bit more, if we can store items as some kind of static variable on the widget, we can drop staleItems (and findStaleItems) altogether. Because items === staleItems when switching back to a quick access panel.

What do you think of:

  1. A single object in the base abstract widget’s closure.

    const staleItems = {};
    
    createWidget("quick-access-panel", {
      html() {
        const items = staleItems[this.key] || [];
      }
    });
    
  2. Emulate static variable with a property on a widget method, like:

    createWidget("quick-acces-panel", {
      staleItems() {},
    
      html() {
        const items = this.staleItems[this.key] || [];
      }
    });