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

Move notifications bell back in labs #11508

Merged
merged 13 commits into from
Sep 15, 2023
Merged

Conversation

germain-gg
Copy link
Contributor

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

For element-hq/element-web#25883

Fixes: https://github.com/vector-im/element-internal/issues/444

To review alongside element-hq/element-web#25924

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)

This change is marked as an internal change (Task), so will not be included in the changelog.

@germain-gg germain-gg requested a review from a team as a code owner August 31, 2023 20:23
@germain-gg germain-gg added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Aug 31, 2023
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Tested and works with the new room header. 👍

Should it also work with the existing one for the transitioning phase? Otherwise there would be a labs setting, that doesn't seem to be working.

image

"feature_notifications": {
isFeature: true,
labsGroup: LabGroup.Messaging,
displayName: _td("labs|notifications"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think notifications is a bit too generic here. What about notifications_panel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the current terminology we refer this piece of UI with. I'll defer to @daniellekirkwood for the final wording

Copy link
Contributor

Choose a reason for hiding this comment

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

In translation the name is „Notifications panel“. If I see notifications as a translator I would naively translate it with „Notifications“ / „Benachrichtigungen“ instead of „Notifications panel“ / „Benachrichtigungsbereich“.

Choose a reason for hiding this comment

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

Labs flags in product appear to have the structure: Header, subhead so my suggestion would be this:

Enable the notifications panel in the room header
Unreliable in encrypted rooms

element-hq/element-web#25924

src/components/views/rooms/RoomHeader.tsx Show resolved Hide resolved
@germain-gg
Copy link
Contributor Author

Thank you for the review. I have purposefully not written any tests for LegacyRoomHeader as this will be removed soon.

Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

👍 Thanks

# Conflicts:
#	src/i18n/strings/en_EN.json
#	test/components/views/rooms/__snapshots__/RoomHeader-test.tsx.snap
@MidhunSureshR
Copy link
Contributor

@weeman1337 , re-requesting review just to let you know that I'm going to merge this PR even with sonarcloud failing; from #11495:

I would like to have this pull request go through regardless of the sonarcloud gates passing of notes. HeaderButtons and LegacyRoomHeader are two files bound to be removed after the header migration, and there is no point in writing tests for them at all.

Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Tested, works 👍

@andybalaam andybalaam merged commit 0b50e02 into develop Sep 15, 2023
75 of 76 checks passed
@andybalaam andybalaam deleted the germain-gg/notifications-labs branch September 15, 2023 13:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants