Skip to content
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

Bridge info settings tab #3693

Merged
merged 18 commits into from
Jan 6, 2020
Merged

Bridge info settings tab #3693

merged 18 commits into from
Jan 6, 2020

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Dec 2, 2019

This is a room settings tab that shows the connected bridges in a room, which is my proof of concept on the client side for MSC2346.

image

EDIT: Added a few more details to the page

Paired with element-hq/element-web#11778 - TR

@Half-Shot
Copy link
Contributor Author

Notable changes from today:

  • i18n is now being used
  • Removed active status, since it's been removed from the MSC.

@Half-Shot Half-Shot marked this pull request as ready for review December 9, 2019 13:57
@Half-Shot Half-Shot changed the title WIP: Bridge info settings tab Bridge info settings tab Dec 9, 2019
@Half-Shot Half-Shot requested a review from a team January 2, 2020 10:57
@turt2live turt2live requested review from turt2live and removed request for a team January 2, 2020 16:41
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

The code itself seems fine - it's mostly just linting/standards/ways of doing things. Please also update the screenshot.

@Half-Shot
Copy link
Contributor Author

Note that in 50e19ba I made the change to remove "bot user" from the text. Due to matrix being how it is, we cannot determine if the sender of the state event is a bot user.

To err on the side of paranoia, let's not imply they are a bot user in the text to avoid any weirdness. Users should hopefully be able to distinguish a user from a bot by the displayname/mxid, or at least not be intentionally misled.

Copy link
Contributor Author

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Second thoughts on the setting name


if (shouldShowBridgeIcon) {
tabs.push(new Tab(
_td("Bridge Info"),
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 wonder if this should just be "Bridges", or "Connected Bridges", or "Integrations"

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

otherwise lgtm enough to merge

@Half-Shot
Copy link
Contributor Author

@turt2live we good now?

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

this side lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants