User Notifications Page Filter (PR #9535)

  • Adds a select kit to filter notifications on user notifications page as discussed

GitHub

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/notifications-unread-only-filter/37621/30

Looks like there are still linting errors. Could you resolve those please before having us review?

Looks like there are still linting errors. Could you resolve those please before having us review?

I can see a few files listed by prettier but not the actual issues. How do I find them?

Hi @Ahmedgagan - you should be able to run prettier locally to fix the files.

Hi @Ahmedgagan - you should be able to run prettier locally to fix the files.

I’ve fixed all the linting issues via prettier. I can see a few Qunit tests failing but those are probably unrelated?

@jjaffeux can you review this one? The test failure does not happen locally for me. But I think your knowledge of dropdowns would be useful here.

Yes will review tomorrow.

Im sorry this was too hard to review line by line as I disagree with the general idea. I don’t think we need all this complexity.

I made you a diff of what I would prefer: user-notifications-filter.patch.zip

It would now show like this: Screenshot 2020-04-29 at 13 29 27

It doesn’t rely on a custom select-kit component and prevents multiple issues, I let you test it apply it if you don’t see any problem with this approach, then I can do another review.

Thanks for your review and code @jjaffeux, I understood the changes specified by you, should I use the above mockup or the one discussed here

Didnt know there was a mockup for this :frowning:

Should have been linked in initial description of the PR.

So ok You can try to reuse what I did but overwrite only the header please.

@jjaffeux done as specified, and all issues are solved :slightly_smiling_face:, please do have a look.

Cool, thanks. Will do first thing tomorrow morning.

Please look at my git diff, I showed you how to make it. The /select-kit/components/notifications-filter.js.es6 file is still totally incorrect, look at how I do computed, and how I use actions to update value, and not mutate it in place.

I have replaced mutation in place with actions as you specified

This is incorrect, I showed the correct way in the diff

You should just use the defaults name and id as shown in my diff

This need to be translated

We don’t need this line as show in my diff