FEATURE: Move sticky avatars into core (PR #14739)

:construction: :construction: :construction:

GitHub

  _handleScroll(offset) {

?

        .querySelectorAll(`${this.topic_post_selector}.${this.stickyClass}`)
        .querySelectorAll(`${this.topicPostSelector}.${this.sticky_class}`)

:+1: Using @bind it works indeed.

do we use this._conainter somewhere?

To be honest I’m kinda mixed on the use of a separate class here, I fail to see the value it adds in this case.

Nope I don’t think so. This PR is a WIP though, still have a lot of things to change and/or try :wink:

I would put this directly in initializer, we want to avoid doing anywork or creating object as soon as possible

To be honest I’m kinda mixed on the use of a separate class here, I fail to see the value it adds in this case.

Well encapsulation, separation of concerns, that sort of things? Maybe I’m too much used to it with Ruby but I think extracting related code into its own class is actually always beneficial. Or maybe what you’re saying is not about putting all this code in its own file but just its own class? Like we could simply use a module with the same functions defined and import it and call it from the initializer?

To be honest I’m kinda mixed on the use of a separate class here, I fail to see the value it adds in this case.

Well encapsulation, separation of concerns, that sort of things? Maybe I’m too much used to it with Ruby but I think extracting related code into its own class is actually always beneficial. Or maybe what you’re saying is not about putting all this code in its own file but just its own class? Like we could simply use a module with the same functions defined and import it and call it from the initializer?

Yes but we are not actually separating here. I think the initializer by itself is a separation, it’s a class dedicated to this task of decorating a topic with sticky avatars. It’s not like we are adding sticky avatar code in the middle of post stream widget for example.

We could indeed, thus it means the initializer “knows” sticky avatars shouldn’t be applied on mobile. That’s one of the point of using a class IMO, this kind of responsibility is encapsulated in it.

To be honest I’m kinda mixed on the use of a separate class here, I fail to see the value it adds in this case.

Well encapsulation, separation of concerns, that sort of things? Maybe I’m too much used to it with Ruby but I think extracting related code into its own class is actually always beneficial. Or maybe what you’re saying is not about putting all this code in its own file but just its own class? Like we could simply use a module with the same functions defined and import it and call it from the initializer?

Yes but we are not actually separating here. I think the initializer by itself is a separation, it’s a class dedicated to this task of decorating a topic with sticky avatars. It’s not like we are adding sticky avatar code in the middle of post stream widget for example.

I really don’t think we should define code in an initializer, to me it’s really just a place where you configure stuff at the boot of the app. And that’s what’s explained in the Ember docs actually: Initializers - Application Concerns - Ember Guides :sweat_smile: Re-reading this it even seems we should probably hook our code somewhere in a route and/or maybe create a component :thinking:

The title of this pull request changed from “WIP: Move sticky avatars into core” to "WIP: FEATURE: Move sticky avatars into core

This is extremely minor, but after living with this for a bit, I wonder whether it would be better to remove the first part of this OR clause, this.direction === "⬆️", that way whether you are scrolling up or down, the same avatars would be sticky.

It was done like that originally actually :slightly_smiling_face: Only long enough posts were sticky but when scrolling up it’s not always clear who’s the author (even with small posts) so we decided to make all posts sticky when scrolling up.

For whatever reason it seems the ember container isn’t always at the top when running this test so it breaks (on FF).

This event isn’t triggered automatically as it should be so we need to manually trigger it here to make the test pass. I guess it’s related somehow with the fact we’re observing window for scroll events instead of the ember container (not sure if this is fixable at some point).

Doesn’t work without later and it wasn’t green on CI with 10 ms so we tried with 100 ms :grimacing: Seems to be working now. At some point we should probably try to understand what’s the root cause (async stuff for sure :sweat_smile:)

The title of this pull request changed from “WIP: FEATURE: Move sticky avatars into core” to "FEATURE: Move sticky avatars into core