FEATURE: Move site updated modal into a less obtrusive prompt (PR #12577)

discourse
#1

GitHub

#2

@eviltrout I requested a review from you here because I added this part to helpers/component-test.js

this.registry.register("message-bus:main", MessageBus, {
    instantiate: false,
});
this.registry.injection("component", "messageBus", "message-bus:main");

I have done this because the widget subscribes to a message bus channel and I need to simulate this channel receiving a message in my test:

// Mimic a messagebus message
MessageBus.callbacks
  .filterBy("channel", "/global/asset-version")
  .map((c) => c.func("somenewversion"));

I am not really sure if the message bus was intentionally left out as a service to inject into the tests? I am unfamiliar with how this stuff works in Ember/our setup.

#3

That won’t fly with almost all themes, forcing theme authors to override .software-update-prompt. It would be better to position the prompt relative to the header.

#4

Oh, and looking at the mobile screenshot I see the dreaded content bleed issue, visible between the prompt and the header. I think it’s a browser bug, floating point precision error or something, but it looks like it’s here to stay so we can work around it by displaying the prompt behind the header (z-index) and stretching it slightly in the upward direction (so: moving it a pixel or two higher and increasing top padding the same amount)

#5

I’m not sure how I feel about having it in the DOM at all times :eyes: But I guess it makes animating it easier.

#6

It definitely was not intentional. If it’s present in the application it should be present in tests.

#7

Yeah I think both of these issues would be solved by moving the prompt to be right after .d-header (but still within .d-header-wrap). @martin-brennan if you can move it there I can go ahead and take care of the styling

#8

@awesomerobot okay this is ready for your once-over now!

#9
#10
#11

This pull request has been mentioned on Discourse Meta. There might be relevant details there: