FEATURE: Allow selective dismissal of new and unread topics (PR #12976)

Also show top dismiss button based on whether the bottom one is in the viewport, not just topic count.

GitHub

unrelated: I wonder if we should clean up usages of this API so we never call it with jQuery objects, seems like a superfluous test, just send it the right type of object.

will this end up with a 500 error if you do topic_ids=1 ? do we want to ensure it is an array?

do we want to allow for duplicates and race conditions here?

server side looks good minus a few minor comments, @CvX can you review the client side?

reads() may be better suited here

    if (position !== "top") {
      return true;
    }

    return !(isOtherDismissUnreadButtonVisible || isOtherDismissNewButtonVisible);
  @afterRender
  _determineOtherDismissVisibility() {
    if (this.position === "top") {
      this.set(
        "isOtherDismissUnreadButtonVisible",
        isElementInViewport(document.getElementById("dismiss-topics-bottom"))
      );
      this.set(
        "isOtherDismissNewButtonVisible",
        isElementInViewport(document.getElementById("dismiss-new-bottom"))
      );
    } else {
      this.set("isOtherDismissUnreadButtonVisible", true);
      this.set("isOtherDismissNewButtonVisible", true);
    }

(needs: import { afterRender } from "discourse-common/utils/decorators";)

Looks like a job for or()

    return new RegExp(filterType + "$", "gi").test(filter);

Would this work?

    let { tracked, tag, topicIds } = {
      tracked: false,
      tag: null,
      topicIds: null,
      ...opts
    };

@CvX afterRender did not work for me at all here for some reason, the code inside the afterRender function was never executed?

🤦 Gah, my bad! I always forget how @afterRender works! It’s not “run this code after rendering” but “when called, delay the execution after rendering”. So the correct suggestion would be either:

  didInsertElement() {
    _super.didInsertElement(...arguments);

    // code

or:

  init() {
    this._super(...arguments);
    this._determineOtherDismissVisibility();
  }

  @afterRender
  _determineOtherDismissVisibility() {
    // code

Agreed, removing this

@SamSaffron based on the docs insert_all (ActiveRecord::Persistence::ClassMethods) - APIdock and other usages of insert_all in our code, it skips duplicates by default, so I don’t think there is anything we need to do here.

1 Like

Cool, will just leave it as is then :slight_smile:

@SamSaffron and @CvX can you both please take another look at this, I have addressed the review comments.

Is the later() call required though?

As far as I could tell, yes. If I removed it I did not get the desired result — the other element is not counted as visible even though it clearly is.