Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ RadoBoiii
✅ jelveh
❌ Rishabh Shinde

Hi, thanks for opening this. Does this PR replace #1189? I'll do a manual test later today.

Can this be merged to the repository?

Just adding a note here since we talked outside the repo - this will be merged pending further review from @jelveh

I just did a manual test. The behavior of displaying notifications seems to be perfect now. I noticed only two issues:

• minor cosmetic issue: contrast of the bell sometimes makes it difficult to see. I'm gonna find out how @jelveh thinks it should look.
• clicking notifications from the panel has different behavior than clicking notification toasts; it's better if it doesn't do anything at all. We can always add functionality later, but we should avoid broken behavior because it decreases the user's confidence in the system.

This is what happens if I click a file share notification from the panel instead of clicking the notification toast; I get this strange broken window:

Manual Test Recording

Hi, I fixed the issues that you addressed. I am able to see the bell icon perfectly. I don't understand why you are unable to see it. Can you check this PR?

I just did a quick test:

• I see the issue with clicking notifications has been fixed.
• I noticed style changes in the top bar. These were present before but I hadn't noticed them.

I think what you're trying to do here is simplify the CSS/DOM of the top bar so that we can more easily add icons while ensuring consistency. I agree that this is necessary, but the appearance of the top bar should not change as a result. If the notification panel doesn't depend on these changes, perhaps it's more appropriate as a separate PR.

Padding of icons changed

This is what the padding looks like in this PR:

This is what it looks like on main currently:

The layout in this PR adds more space between icons, causing the icons to take up more space on the top bar.

Profile icon

This is probably related to the padding issue mentioned above. The profile image area gets moved to the top-left of its container, making the UI look broken.

The notification panel does not depend on these changes. What should I do for the layout? Should I try to fix it or will it be considered as another PR?

The notification panel does not depend on these changes. What should I do for the layout? Should I try to fix it or will it be considered as another PR?

I'm find with either, but we can likely merge the notification panel sooner if the other changes are moved to a separate PR

Sure then, let's merge it and I can try to fix it as another PR

Sure then, let's merge it and I can try to fix it as another PR

What I meant was the CSS changes made in the PR could be moved to another, and that would expedite merging this. I can't merge a PR with CSS regressions on components of the desktop UI.

Does this look better?

There are still styles changes. I want all the style changes for the toolbar removed - they don't need to be in this PR. That means comparing with the style on main and making sure this PR doesn't change it. I'll send a patch file shortly that will make the required changes with a git apply.

Well, I was going to send a .diff or .patch file but, shockingly, GitHub doesn't support these file extensions; so instead, here's a .txt file which really is a diff.

You can apply this with git apply path/to/pr-1240_rm-toolbar-css.diff.txt

pr-1240_rm-toolbar-css.diff.txt

This diff removes the toolbar style changes and makes it consistent with the current style on main. This will make it possible to merge this PR. Right now (prior to applying this diff) the style changes have visual impacts such as making the toolbar icons further apart from each other; this may seem trivial but it's not and it would tie a lot of unrelated concern to this notification panel PR which you wouldn't want to deal with here - definitely better to put those changes in another PR.

I have run the command and those changes have been applied to the repository

Perfect, thanks! I'm going to ask @jelveh if he wants to review the UI first but otherwise this looks ready to merge to me

I think @jelveh has tested it, is it ready to be merged?

Hey @KernelDeimos can this PR be merged or does it require additional changes

Any updates on this @KernelDeimos ?

Any updates on this @KernelDeimos ?

Not yet, we've got a lot on our plates at the moment.