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

Add notification quick-filter bar in the frontend app #9399

Merged
merged 26 commits into from Dec 16, 2018

Conversation

Projects
None yet
10 participants
@pawelngei
Contributor

pawelngei commented Nov 30, 2018

Implements #7123 .

samplefilterbar

The quick-filter bar is toggleable in settings and visible by default in the Notifications column. The filtering options are ignored when it's turned off.

It filters specific types of notifications (one at a time), even if that type is absent from the general view due to the user's other settings.

EDIT: Polished the code and added Polish translations ;)

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Nov 30, 2018

I addressed the eslint problems and added Polish and English translations. Please let me know if I should add the translation keys anywhere else.

The PR should be ready to merge.

I can add unit tests in a separate PR not to clutter this one.

@Gargron

This comment has been minimized.

Member

Gargron commented Nov 30, 2018

Thoughts:

  • How about reusing the design of the "Toots"/"Toots with replies" tab bar from profiles
  • How about simplifying it to "All" and "Mentions"? Not sure there is a lot of demand for the others
@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Nov 30, 2018

  • I thought about the design for some time and I think that @ThisIsMissEm 's mockup looks much cleaner than "Toots / Toots with replies / Media". That's my preference and if you'd like to change it and have one unified look or as little different CSS as possible, I can redesign it.
  • "All" and "Mentions" are the most important categories, but could personally use "Boosts" and "Follows" myself. After writing one toot which proved very popular, I really missed the ability to see all the people who followed me. The only drawback (besides giving users too many options) I can think of is that these categories can fall into two rows for some languages.

Expanding the second point, I think grouping the notifications as mentioned in #1483 could work even better, but for now I would opt for leaving these two as they are.

EDIT: Also, did I miss any locale files? I couldn't find anything on how these strings should be handled in the developer's docs, so I just went with two .json files.

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Nov 30, 2018

I created a fork of the code with a @Gargron 's suggested unified design: https://github.com/pawelngei/mastodon/tree/filter-bar-unified-look

unified_style

The drawback is that it doesn't look that well multiline:

multi-line-unified

@Cassolotl

This comment has been minimized.

Cassolotl commented Nov 30, 2018

This looks great, I hope it gets merged! Maybe a little more space above the words? :)

more space

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 2, 2018

I think we can remove work-in-progress tag. This version is ready, as is the redesigned one. The question is: do we want five categories (the initial design suits it better), or only two/three (the unified one looks okay even for longer languages)?

@trwnh

This comment has been minimized.

Contributor

trwnh commented Dec 3, 2018

If there's a concern about overflows, why not simply use the icons? They should be very straightforward:

  • asterisk = all
  • at-sign = mentions
  • star = favorites
  • retweet = boosts
  • contact-plus = follows
@ThisIsMissEm

This comment has been minimized.

Contributor

ThisIsMissEm commented Dec 3, 2018

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 3, 2018

Actually, icons don't look that bad, especially if we use tooltips explaining them:

screenshot at 2018-12-03 08-59-01

With longer languages like this:

icons-pl

Tooltip example:

icons-tooltips

Everything is implemented at https://github.com/pawelngei/mastodon/tree/filter-bar-icons

@bannisdale

This comment has been minimized.

bannisdale commented Dec 3, 2018

The icons look great and are also used throughout the UI already. Love it! :)

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 3, 2018

@ThisIsMissEm , I see three options now:

  1. We implement your initial design, which is a little bit different than the standard.
  2. We cut out some of the categories (I would not like that myself).
  3. We use the icons with the unified design.

What do you think would be the best version? Can we accept the line overflowing into the next row?

I'm currently supporting the 3rd option the most. If you agree that icons with tooltips are readable (since there's a huge "Notifications" for context above), I can merge the changes and we can ask the devs to review this PR.

@ThisIsMissEm

This comment has been minimized.

Contributor

ThisIsMissEm commented Dec 3, 2018

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 5, 2018

Text replaced with an icon.

If we use icons and there is no risk of overflowing, I will argue that more filters are better than fewer. Personally, I could really use those to count / inspect people who fav and boost my toots.

I think that we can gather some feedback from actual users once several instances use the feature - then we will know if there are any confusing parts. Hopefully, we will also have some specific suggestions with mockups.

@krainboltgreene

Code looks good, but I can't review or merge this one since I'm not a part of that process currently.

@nightpool

This comment has been minimized.

Collaborator

nightpool commented Dec 8, 2018

I agree with @ThisIsMissEm that icons are very confusing for users.

I also agree with @Gargron that this is a really complex UI to throw at new users, and most people would be better served by only having "All" and "Mentions" (and maybe "Follows") tabs.

@nightpool

This comment has been minimized.

Collaborator

nightpool commented Dec 8, 2018

I think these two concerns are also intersecting—I could support a design that had one or two icons, instead of 5.

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 8, 2018

Since being able to use all 5 is very important to me, I could propose two solutions:

  1. We show only "All" and "Mentions" (text) by default, and users could turn on "Advanced mode" with all 5 in the notifications settings. This way we don't confuse new users while still allowing others to use all the quick filters.

  2. Alternatively, we show only "All" and "Mentions" by default, but allow to turn each one on and off in specific category Notification settings. This will allow more granular control, but be more confusing.

@nightpool @Gargron what do you think about it? I'll give you mockups / code in a moment.

@bannisdale

This comment has been minimized.

bannisdale commented Dec 8, 2018

@pawelngei I like both approaches a lot. :)

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 8, 2018

Added the simple mode with only "All" and "Mentions", visible by default:

quick_filter_simple_view

As well as the optional "advanced" mode with all 5 categories:

quick_filter_advanced_view

@Gargron

I do not see the code affecting the server-side query, so pagination is going to be wonky with any of these filters enabled. For example, if you only view mentions, and there is a page of follows, all are going to be invisible, and the next page won't load. See how the other toggles work in this column

@nightpool

This comment has been minimized.

Collaborator

nightpool commented Dec 9, 2018

@nightpool

This comment has been minimized.

Collaborator

nightpool commented Dec 9, 2018

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 9, 2018

@nightpool "All" shows the filters that you set in the notification column, so if you unticked Follows, you won't see them there. Each separate category will always show only what's in there, independent of your Notification options.

@Gargron Thank you, I'll write the queries tomorrow.

@LoVicente

This comment has been minimized.

LoVicente commented Dec 11, 2018

hi, I have a question regarding this: Will the tabs have any indications that there are new elements within them? or will I have to constantly toggle between them to check? Don't get me wrong, I think something like this is sorely needed but this is a new type interface that differs from the current columns approach where things are always being shown.
Also, does this feature mean that notifications grouped by type (a la twitter) will not happen?

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 11, 2018

@LoMakesComics thanks for asking these questions!

The end result depends mainly on maintainers and their vision, but I was planning to finish this PR without implementing per-type notifications, as that would require more Ruby knowledge than I currently have. We would need to change the API and need to introduce a "notification read" system unified among different Mastodon clients (if I understand it correctly).

I wanted to tackle the "group notifications" next, but I think that creating the indicators may be a better, smaller task after this ticket. I will prepare some sketches of API and the interface in a separate issue.

I can't see how implementing separate filters could affect grouping negatively: even within one type of notification, it's great to see "5 people boosted post X" and "3 people boosted post Y".

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 12, 2018

@Gargron I have tweaked with the filters and I have two questions:

  1. Should we save the active filter in the Settings? If not, where else should it lie in Redux?
  2. Should we remove the notifications downloaded so far when going between the filters? Assuming that we'll use the param exclude_types, I don't want us to end up with all notifications 1-15 and then 24, 27, 45, 71 which happen to be mentions.

I'd rather remove the showFilterBar and allowedType from getNotifications, and flush all notifs each time the user changes a filter. What do you think about it?

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 12, 2018

Okay, I created a new action and handled the pagination. Now this feature is a little bit heavier, but I think it deals with all the edge cases.

Oh, and showFilterBar and allowedType turned out to be required in order not to "turn off" some of the notification categories with other settings.

@pawelngei

This comment has been minimized.

Contributor

pawelngei commented Dec 12, 2018

The end effect:

notifications gif

@Gargron Gargron merged commit 13dce12 into tootsuite:master Dec 16, 2018

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment