REFACTOR: Use IntersectionObserver to calculate topic progress position (PR #14698)

GitHub

I checked our all-the-plugins/themes repos and didn’t find an instance of this event being used. It’s also not a good idea to use it because it would likely be jittery, since core would have already done and applied its changes.

If I understand correctly, the relit gem will automatically flip the right/left position values. I’m not sure whether it does the same for flex properties, hence I added this here. Will confirm.

No longer need to save this extra space, because when it’s not position: fixed, the progress element reserves its own space.

    this._topicBottomObserver?.disconnect();

We got to find a way to achieve this without this pattern

Thanks for the review, I’ve pushed a better way to do this (using @bind).

@OsamaSayegh do you have any thoughts about this?

The rtlit gem doesn’t flip flex-start:

➜  rails c
Loading development environment (Rails 6.1.4.1)
[1] pry(main)> require 'r2'
=> true
[2] pry(main)> R2.r2("a { justify-content: flex-start }")
=> "a{justify-content:flex-start;}"
[3] pry(main)> R2.r2("a { padding-right: 1em }")
=> "a{padding-left:1em;}"

I believe this behavior is expected since “start” does not necessarily mean left or right; the browser will figure out what do based on the dir attribute in the HTML. If dir="rtl", then justify-content: flex-start will align the element to the right, otherwise the element is aligned to the left.

Here is a quick demo:

<!DOCTYPE html>
<html>
  <head>
    <style>
      .parent { width: 100%; display: flex; justify-content: start; }
      .child { background: blue; color: white; padding: 1em; margin: .5em; }
    </style>
  </head>
  <body>
    <p>LTR:</p>
    <div class="parent">
      <div class="child">One</div>
      <div class="child">Two</div>
      <div class="child">Three</div>
    </div>
    <p>RTL:</p>
    <div class="parent" dir="rtl">
      <div class="child">One</div>
      <div class="child">Two</div>
      <div class="child">Three</div>
    </div>
  </body>
</html>

So as long as we include dir="rtl" on a top-level element (e.g. <body>) when we render an RTL layout, I don’t think we need this specific CSS rule.

Thanks, @OsamaSayegh!

are you sure you want alias here? Alias is generally a big footgun which I don’t recommened, reads is probably a better choice in most cases.

is this still need actually needed ?

oh cool didn’t know about removeProperty usually would do = null :+1:

I think you could experiment with creating the observer in did insert and just call observe() again if you need it, might need to call unobserve on previous node. Don’t know enough the problem at hand but, it seems like we are going with the tank solution of, and we could maybe have something lighter.

The reason why we need to disconnect and reinitialize the observer is because we need to account for the composer height (when it’s half size, resized or minimized), but the IntersectionObserver API does not support a dynamic rootMargin value.

It looks like support for a dynamic rootMargin is coming: Allow dynamically changing rootMargin · Issue #428 · w3c/IntersectionObserver · GitHub, so I’ll leave a note to that effect.

Ok cool then :+1:

I was about to remove it, then I remembered this: DEV: Remove IntersectionObserver polyfill by pmusaraj · Pull Request #13445 · discourse/discourse · GitHub, so, yeah, we should keep it for these very rare cases (sadly).