Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Move notification bell to space panel #11373

Closed
wants to merge 8 commits into from

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented Aug 4, 2023

For https://github.com/vector-im/internal-planning/issues/444

Screenshot 2023-08-04 at 15 11 56

Moves the notification button in the space panel and adds it to labs as it currently only works with unencrypted content. We would consider getting it out of labs when it works meeting the basic expectations of our userbase, i.e. works with all events.

@nadonomy we need to coordinate on the announcement about this being labsed
@daniellekirkwood could you review the copy of the betaInfo (in `Settings.tsx) please?

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

  • Move notification bell to space panel (#11373).

@germain-gg germain-gg added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Aug 4, 2023
@germain-gg germain-gg requested a review from a team as a code owner August 4, 2023 14:32
src/hooks/useGlobalNotification.ts Outdated Show resolved Hide resolved
src/settings/Settings.tsx Show resolved Hide resolved
src/settings/Settings.tsx Outdated Show resolved Hide resolved
controller: new ReloadOnChangeController(),
betaInfo: {
title: _td("Notifications panel"),
caption: () => <p>{_t("A list of all events notifying you. Only works with unencrypted rooms")}</p>,
Copy link
Member

Choose a reason for hiding this comment

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

Only works with unencrypted rooms

This isn't true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's desirable to be overly specific here. I'd be open to hearing alternative wording though.

This is how everyone internally at Element has described to me how this feature works. I'm trying to avoid being technically overly accurate to not overwhelm a more average user that might not grasp all the tiny distinctions between the concepts discussed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looping @daniellekirkwood and @nadonomy here

Choose a reason for hiding this comment

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

Yeah, my understanding is that it only works with Unencrypted rooms so this feels accurate.

Copy link
Member

@t3chguy t3chguy Aug 7, 2023

Choose a reason for hiding this comment

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

The /notifications API in the spec does not discriminate whether a room is encrypted or not. The actual matching is for events which match set_tweak highlight in your push rules. So if you disable Noisy notifications it won't even include unencrypted rooms.

src/settings/Settings.tsx Outdated Show resolved Hide resolved
default: false,
// Reload to ensure that the left panel etc. get remounted
controller: new ReloadOnChangeController(),
betaInfo: {
Copy link
Member

Choose a reason for hiding this comment

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

Marking this as a beta implies its nearly ready for prime time, by taking this feature out of mainline stability we're saying it isn't. This will be confusing IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only way to make it available to people who do not use develop/nightly.
I don't think this will be more confusing than the way it works today

src/components/views/spaces/SpacePanel.tsx Outdated Show resolved Hide resolved
src/components/views/spaces/SpacePanel.tsx Outdated Show resolved Hide resolved
@t3chguy
Copy link
Member

t3chguy commented Aug 4, 2023

CI is also very unhappy

@t3chguy
Copy link
Member

t3chguy commented Aug 4, 2023

Screen.Recording.2023-08-04.at.16.08.48.mov

Also does not seem to work :/

@germain-gg germain-gg marked this pull request as draft August 7, 2023 06:29
@germain-gg germain-gg removed the request for review from florianduros August 7, 2023 06:29
@germain-gg germain-gg added T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements and removed T-Task Refactoring, enabling or disabling functionality, other engineering tasks labels Aug 7, 2023
@daniellekirkwood
Copy link

@nadonomy should we default the labs flag to on so it's not so much of a who-moved-my-cheese all at once?

@germain-gg I imagine we'll need an in-product announcement as well as a coordination in rooms and release notes. Would like Nad's thoughts on that...

@germain-gg
Copy link
Contributor Author

@germain-gg germain-gg closed this Aug 22, 2023
@germain-gg germain-gg deleted the germain-gg/labs/notifications branch August 31, 2023 20:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants