Skip to content

Sort persistent notifications ascending#7195

Merged
bramkragten merged 4 commits intohome-assistant:devfrom
Misiu:sort_notifications_ascending
Oct 17, 2020
Merged

Sort persistent notifications ascending#7195
bramkragten merged 4 commits intohome-assistant:devfrom
Misiu:sort_notifications_ascending

Conversation

@Misiu
Copy link
Copy Markdown
Contributor

@Misiu Misiu commented Oct 2, 2020

Breaking change

Proposed change

sort persistent notifications, to show the most recent on top.

The sort is done in code, the other option is to use https://polymer-library.polymer-project.org/3.0/api/elements/dom-repeat#DomRepeat-property-sort, but notification-drawer will be rewritten someday to Lit, so the first option was easier.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Co-authored-by: Bram Kragten <mail@bramkragten.nl>
@Misiu
Copy link
Copy Markdown
Contributor Author

Misiu commented Oct 2, 2020

@bramkragten should I add undefined check as you suggested here: #7199 (comment)

@bramkragten
Copy link
Copy Markdown
Member

Should I add undefined check as you suggested here: #7199 (comment)

Depends on the type, can it be undefined?

@ludeeus
Copy link
Copy Markdown
Member

ludeeus commented Oct 2, 2020

Instead of the logic added here, and more or less the same logic in #7199, shouldn't we rather make a new helper for compareDatetime?

@Misiu
Copy link
Copy Markdown
Contributor Author

Misiu commented Oct 5, 2020

Depends on the type, can it be undefined?

According to the interface, it can't be undefined.
Ref:

export interface PersistentNotification {
  created_at: string;
  message: string;
  notification_id: string;
  title: string;
  status: "read" | "unread";
}

@Misiu
Copy link
Copy Markdown
Contributor Author

Misiu commented Oct 16, 2020

@Misiu
Copy link
Copy Markdown
Contributor Author

Misiu commented Oct 16, 2020

Something like this?

function SortByDateAscending<T>(
  values: T[],
  getDate: (val: T) => string | number | Date
) {
  const compare = (first: T, second: T) => {
    const d1 = getDate(first) ?? 0;
    const d2 = getDate(second) ?? 0;
    const timeA = d1 ? new Date(d1) : 0;
    const timeB = d2 ? new Date(d2) : 0;
    if (timeA < timeB) {
      return 1;
    }
    if (timeA > timeB) {
      return -1;
    }
    return 0;
  };
  return values.sort(compare);
}

And usage looks like this:
tokens = SortByDateAscending(tokens, token => token.last_used_at);

@bramkragten
Copy link
Copy Markdown
Member

The helper function can be done in a new PR.

@bramkragten bramkragten merged commit 288bf68 into home-assistant:dev Oct 17, 2020
@bramkragten bramkragten mentioned this pull request Oct 21, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change sort order of Persistent Notifications (show most recent notifications at top)

5 participants