Help us test the new header code!

feature
63

I’ve taken a first pass at updating Quick Messages to the new header logic

@eviltrout Some issues / questions / thoughts:

  1. Could you break out the avatarImg method from the post widget? It seems to be a useful general way to include avatars in widgets.

  2. It’s a little inconvenient to add new menus to the header widget because the widget contents are wrapped inside html in the render function. I ended up adding the quick message menu itself as another list item after the quick message icon.

    edit I’m probably missing something, but there seems to be a similar issue for the Header Search Plugin. If I insert it after the home-logo widget, i.e. api.decorateWidget('home-logo:after' ... it ends up inside the title element, which leads to conflicts with the home-logo click event…

  3. I’m wondering what the point of having widget events as separate from widget actions is? i.e. when would you use an event instead of an action?

  4. Following your lead with the _notificationsChanged function, I put my observers for messages in the site-header component and then re-rendered all of the header widgets when the observer fires. This pattern doesn’t feel amazing. Thoughts on best practices in this kind of situation?

Cheers,

Angus

3 Likes
64

So the way I ended up handling this for now is by overriding the ‘click’ event in the home-logo and popping the routeTo function in a condition that checks if the target is the site logo

      api.attachWidgetAction('home-logo', 'click', function(e) {
        if (wantsNewWindow(e)) { return false; }
        e.preventDefault();
        if (e.target.id === 'site-logo') {
          DiscourseURL.routeTo(this.href());
        }
        return false;
      })

ps. I’ve finished widgetizing the header-search plugin:

4 Likes
65

Would it be possible to create a widget and attach it before another using decorateWidget so that you can put your own click handler in there?

66

  1. Sure I think that’s a good idea. I’m in the middle of other work right now but if you did a PR for that I’d accept it :slight_smile:

  2. What we could do is add a new decorator target in this case. Something like header:afterPanels. Then it could be added that way.

  3. When you say events are you talking about browser events like click or the app-events? Both exist for different reasons.

  4. It’s a serious downside of the widget approach - they are meant to only update when you interact with them. This is a pattern that came out of creating it for our post stream, where the vast majority of rendering only ever had to happen following a click of some sort. If your data is coming in via a different mechanism (say our message bus) I recommend you use the app-events to trigger a rerender. If your data comes in via interacting with the header you shouldn’t need to do anything. The observer thing was mostly there to simplify and reuse the previous code. If I coded it from scratch I would probably just use app-events for it.

4 Likes
67

@eviltrout do you have any plans for the addition of user menu items?

I’m finding mobile users are having issues navigating and wanting the sites custom menu links back.

68

If you checkout the latest version of Discourse you can now add links to the general section like this:

<script type="text/discourse-plugin" version="0.4">
  api.decorateWidget('hamburger-menu:generalLinks', () => {
    return { href: '/users/eviltrout', rawLabel: 'evil trout' };
  })
</script>
8 Likes
69

Currently this doesn’t work for domains different to the Discourse install.

So for a setup where:

  • Discourse is installed at: http://discourse.example.com/
  • And the blog / homepage is at: http://www.example.com/

Using this code to add a non-Discourse instance domain link:

<script type="text/discourse-plugin" version="0.4">
  api.decorateWidget('hamburger-menu:generalLinks', () => {
    return { href: 'http://www.example.com/sample-page/', rawLabel: 'Example' };
  });
</script>

Links to (when clicked): http://discourse.example.com/sample-page/
instead of: http://www.example.com/sample-page/

The HTML is correct, so I’m guessing the click handler does something funky.

EDIT: Thank you so much for adding this! :fonzie:

EDIT #2: I attempted to add permalinks to the Discourse instance to work around this and redirect to the correct external link. This however didn’t work it appears permalinks are not followed by the menu links, however permalinks are followed when the link appears in post content.

1 Like
70

Fixed here:

https://github.com/discourse/discourse/commit/e8312038974f30df82da080f8c7acfc4d2a1c4e6

4 Likes
71

Yay! Thanks - can confirm links to external sites work.

Although it’s not important to me, I also tested a link to a Discourse “permalink”, this simply displayed the standard “not found” page.