User Notifications Page Filter (PR #9535)

only the value should be passed as value not the while filter object

this should probably just be label

You can probably ditch the > 0 in both cases

if (0) {
  console.log("foo"); // won't be called
}

value should be a localisable string, it should be kinda unique

@jjaffeux You’ve referred to your diff a couple of times. Thanks for going ahead and sharing it. But I think there’s a small problem. Here are the contents of your diff.

diff --git a/app/assets/javascripts/discourse/app/templates/user/notifications-index.hbs b/app/assets/javascripts/discourse/app/templates/user/notifications-index.hbs
index 0862699e43..0483811f28 100644
--- a/app/assets/javascripts/discourse/app/templates/user/notifications-index.hbs
+++ b/app/assets/javascripts/discourse/app/templates/user/notifications-index.hbs
@@ -8,7 +8,7 @@
   </div>
 {{/if}}
 
-{{notifications-filter action=(action "filterNotifications")}}
+{{notifications-filter value=filter onChangeFilter=(action "filterNotifications")}}
 <span class="user-notifications-filter-separator"></span>
 
 {{#if hasNotifications}}
diff --git a/app/assets/javascripts/select-kit/components/notifications-filter.js.es6 b/app/assets/javascripts/select-kit/components/notifications-filter.js.es6
deleted file mode 100644
index 65fa59d14d..0000000000
--- a/app/assets/javascripts/select-kit/components/notifications-filter.js.es6
+++ /dev/null
@@ -1,40 +0,0 @@
-import DropdownSelectBoxComponent from "select-kit/components/dropdown-select-box";
-
-export default DropdownSelectBoxComponent.extend({
-  classNames: ["notifications-filter"],
-  content: [
-    {
-      type: "all",
-      value: I18n.t("user.user_notifications.filters.all")
-    },
-    {
-      type: "read",
-      value: I18n.t("user.user_notifications.filters.read")
-    },
-    {
-      type: "unread",
-      value: I18n.t("user.user_notifications.filters.unread")
-    }
-  ],
-  value: {
-    type: "all",
-    value: I18n.t("user.user_notifications.filters.all")
-  },
-  isVisible: true,
-  valueProperty: null,
-
-  modifyComponentForRow() {
-    return "notifications-filter/notifications-filter-row";
-  },
-
-  selectKitOptions: {
-    headerComponent: "notifications-filter/notifications-filter-header"
-  },
-
-  actions: {
-    onChange(filter) {
-      this.set("value", filter);
-      this.attrs.action && this.attrs.action(filter.type);
-    }
-  }
-});
diff --git a/app/assets/javascripts/select-kit/components/notifications-filter/notifications-filter-header.js.es6 b/app/assets/javascripts/select-kit/components/notifications-filter/notifications-filter-header.js.es6
deleted file mode 100644
index 16fdf1a461..0000000000
--- a/app/assets/javascripts/select-kit/components/notifications-filter/notifications-filter-header.js.es6
+++ /dev/null
@@ -1,13 +0,0 @@
-import DropdownSelectBoxHeaderComponent from "select-kit/components/dropdown-select-box/dropdown-select-box-header";
-import discourseComputed from "discourse-common/utils/decorators";
-
-export default DropdownSelectBoxHeaderComponent.extend({
-  layoutName:
-    "select-kit/templates/components/notifications-filter/notifications-filter-header",
-  classNames: ["notifications-filter-header"],
-
-  @discourseComputed("selectKit.isExpanded")
-  caretIcon(isExpanded) {
-    return isExpanded ? "caret-up" : "caret-down";
-  }
-});
diff --git a/app/assets/javascripts/select-kit/templates/components/notifications-filter/notifications-filter-header.hbs b/app/assets/javascripts/select-kit/templates/components/notifications-filter/notifications-filter-header.hbs
deleted file mode 100644
index a7d21ad317..0000000000
--- a/app/assets/javascripts/select-kit/templates/components/notifications-filter/notifications-filter-header.hbs
+++ /dev/null
@@ -1,7 +0,0 @@
-<span class="filter-text">
-  Filter By
-</span>
-<span class="header-text">
-  {{value.value}}
-</span>
-{{d-icon caretIcon class="caret-icon"}}
diff --git a/app/assets/javascripts/select-kit/templates/components/notifications-filter/notifications-filter-row.hbs b/app/assets/javascripts/select-kit/templates/components/notifications-filter/notifications-filter-row.hbs
deleted file mode 100644
index 83583c08c5..0000000000
--- a/app/assets/javascripts/select-kit/templates/components/notifications-filter/notifications-filter-row.hbs
+++ /dev/null
@@ -1,5 +0,0 @@
-<span class="selection-indicator"></span>
-
-<span class="filter-option">
-  {{rowValue.value}}
-</span>
diff --git a/app/assets/stylesheets/common/base/user.scss b/app/assets/stylesheets/common/base/user.scss
index 869635a250..00ae6bda7f 100644
--- a/app/assets/stylesheets/common/base/user.scss
+++ b/app/assets/stylesheets/common/base/user.scss
@@ -70,6 +70,10 @@
     width: 100%;
     border: 0.5px solid $primary-low;
   }
+
+  .user-notifications-filter {
+    margin-bottom: 0.5em;
+  }
 }
 
 .user-main {
diff --git a/app/assets/stylesheets/common/select-kit/notifications-filter.scss b/app/assets/stylesheets/common/select-kit/notifications-filter.scss
deleted file mode 100644
index a2dd03d3ca..0000000000
--- a/app/assets/stylesheets/common/select-kit/notifications-filter.scss
+++ /dev/null
@@ -1,56 +0,0 @@
-.select-kit {
-  &.dropdown-select-box {
-    &.notifications-filter {
-      display: inline-flex;
-      position: relative;
-
-      .select-kit-collection {
-        padding: 5px;
-      }
-
-      .notifications-filter-header {
-        padding: 0 0.5em 0 0.5em;
-        background: none;
-        border: none;
-        justify-content: flex-start;
-        width: auto;
-        height: auto;
-        span.filter-text {
-          margin-right: 15px;
-          color: $primary-medium;
-        }
-
-        span.header-text {
-          color: dark-light-choose($tertiary, $tertiary);
-        }
-
-        .d-icon {
-          color: $primary-medium;
-          opacity: 1;
-          margin: 5px 0 10px 5px;
-          font-size: $font-up-3;
-        }
-
-        &.is-focused,
-        &:hover {
-          background: none;
-          border: none;
-          outline: none;
-        }
-      }
-      .notifications-filter-row {
-        font-weight: bold;
-        padding: 0.5em;
-        font-size: $font-up-1;
-        align-items: center;
-        display: flex;
-        width: auto;
-        height: auto;
-
-        span.filter-option {
-          flex: 1 0 0px;
-        }
-      }
-    }
-  }
-}

Are we on the same page on this?

@Ahmedgagan Oh damn I didn’t include new files… Why didn’t you say anything first time I told you about this? :smile: Anyways my bad, sorry.

I will make more comments then.

OK will my last comments, you should be good to go :+1:

This file should be .js and not .js.es6

no worries, thanks for helping me out

Ok :+1: looks better, thanks!

Will try it tomorrow and merge if ok.

thanks.

Merged thanks!