Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notification sorting, users table flexible columns #2991

Merged
merged 7 commits into from
Jul 29, 2020

Conversation

rigelk
Copy link
Collaborator

@rigelk rigelk commented Jul 21, 2020

This PR brings two unrelated changes:

  1. Sortable notifications
  • notifications sorting can be changed in the notification view, so as to help display notifications with unread shown first.

Screenshot_2020-07-21 Notifications - PeerTube

  1. Flexible User listing
  • users table could display more information than it currently does ; on small devices - not just mobile - the situation is difficult. Thus being able to toggle columns seems like a good option.
  • statuses in tables are not using labels to provide quickly identifiable information ; I took the label css from primeng's example, which is soft enough to the eye.
  • progress bars were revamped for small column sizes.

Screenshot_2020-07-21 Users list - PeerTube(1)
Screenshot_2020-07-21 Users list - PeerTube

Note: Sorry for the grouped PR - the past weeks have been hectic and I didn't find time to split the features in respective branches.

@rigelk rigelk added UI non-trivial UI changes, that might need discussion Component: Notifications labels Jul 21, 2020
@rigelk rigelk requested a review from Chocobozzz July 22, 2020 06:50
@rigelk rigelk marked this pull request as ready for review July 23, 2020 11:30
Copy link
Owner

@Chocobozzz Chocobozzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests for channels search

client/src/sass/include/_mixins.scss Show resolved Hide resolved
client/src/sass/include/_mixins.scss Outdated Show resolved Hide resolved
@@ -41,9 +41,22 @@ const videoChannelsSearchValidator = [
}
]

const videoChannelsOwnSearchValidator = [
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why OwnSearch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there's already a videoChannelsSearchValidator for public searches, which allows searchTarget as query parameter.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then videoChannelsListSearchValidator?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced

server/models/video/video-channel.ts Outdated Show resolved Hide resolved
@kimsible
Copy link
Contributor

1. Sortable notifications

Great! but on small and mobiles screens, we should adapt the 3 elements :

image

2. Flexible User listing

It's a very cool feature @rigelk ! I love the way it really improve table handling !
I tested it, I have only one thing to say and if I noticed it it's because of this PR :

When you select lots of column the last one become hidden cause to the scroll, so I ask the same question here, would it be good to put all actions including dropdown-actions on the left of user tables to avoid this ?

image

@rigelk rigelk force-pushed the notifications-sort branch 3 times, most recently from 5360f68 to 60979b7 Compare July 24, 2020 09:40
@rigelk rigelk added Status: Requires Tests Either requires manual test, or writing tests, or both and removed Status: Waiting for changes labels Jul 24, 2020
@rigelk rigelk requested a review from Chocobozzz July 24, 2020 09:41
@kimsible
Copy link
Contributor

@rigelk a proposition sortable notifications :

Mobile

image

Small view :

image

Update media queries in

@media screen and (max-width: $mobile-view) {
  .header {
    flex-direction: column;

    & >:first-child, .peertube-select-container {
      margin-bottom: 15px;
    }

    .peertube-select-container {
      margin-left: 0 !important;
    }
  }
}

@media screen and (min-width: $mobile-view) and (max-width: $small-view) {
  .header {
    a {
      font-size: 0;
      padding: 0 13px;
    }

    .peertube-select-container {
      width: auto !important;
    }
  }
}

@media screen and (min-width: $mobile-view) and (max-width: #{$small-view + $menu-width}) {
  :host-context(.main-col:not(.expanded)) {
    .header {
      a {
        font-size: 0;
        padding: 0 13px;
      }

      .peertube-select-container {
        width: auto !important;
      }
    }
  }
}

@rigelk
Copy link
Collaborator Author

rigelk commented Jul 24, 2020

Thanks for the snippet @kimsible, much appreciated 👍

Copy link
Owner

@Chocobozzz Chocobozzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests

@rigelk rigelk requested a review from Chocobozzz July 27, 2020 15:01
@rigelk rigelk removed the Status: Requires Tests Either requires manual test, or writing tests, or both label Jul 27, 2020
@Chocobozzz
Copy link
Owner

tests fail

@rigelk rigelk force-pushed the notifications-sort branch 2 times, most recently from 707b925 to baeeae5 Compare July 28, 2020 11:54
@rigelk rigelk merged commit 7b39096 into Chocobozzz:develop Jul 29, 2020
@rigelk rigelk deleted the notifications-sort branch July 29, 2020 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Notifications UI non-trivial UI changes, that might need discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants