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

Conversation

@estellecomment
Copy link
Contributor

@estellecomment estellecomment commented Oct 21, 2022

Problem solved by this PR :
When no map-server is configured (either in wellknown or in SdkConfig), no location sharing is possible.
Expected : the "Location" button in MessageComposer is not displayed.
Obtained : the button is present, I go through the next clicks, and eventually get an error message that no map server is configured.

This PR removes the "Location" button in MessageComposer when map server is not configured.

Signed-off-by: Estelle Comment estelle.comment@beta.gouv.fr

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

@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Oct 21, 2022
@estellecomment estellecomment marked this pull request as ready for review October 21, 2022 15:53
@estellecomment estellecomment requested a review from a team as a code owner October 21, 2022 15:53
@weeman1337 weeman1337 added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Oct 24, 2022
@weeman1337 weeman1337 requested review from weeman1337 and removed request for SimonBrandner, dbkr and germain-gg October 24, 2022 07:30
@weeman1337
Copy link
Contributor

Thanks for the PR @estellecomment

I will discuss this with design/product.

@weeman1337 weeman1337 self-assigned this Oct 24, 2022
@kerryarchibald
Copy link
Contributor

Related element-hq/element-web#21082

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.

clearing code review while design/product are involved

@turt2live turt2live requested review from a team November 3, 2022 11:36
@jakewb-b
Copy link

jakewb-b commented Nov 3, 2022

I'm not confident this is the right solution.

Having a map server configured is required to view locations (or at least, to view them on a map, which gives them any kind of useful context). It shouldn't be required to send them. It should be entirely possible for a user with no map server configured to send their location, which can be viewed by other users who do have a map server configured.

So, if currently the lack of a map server is blocking users from sending their location, then I would suggest we should fix that, and make it easier for users to still send their location in that circumstance, rather than removing the button entirely.

@estellecomment
Copy link
Contributor Author

So, if currently the lack of a map server is blocking users from sending their location, then I would suggest we should fix that, and make it easier for users to still send their location in that circumstance, rather than removing the button entirely.

That's another solution. It doesn't make sense for our case because our users are in a closed federation so all homeservers have the same features, but I understand that it's not the case in general.

We could also, instead of this PR, add a variable in config that enables/disables the location feature, independently of whether the map server is configured. That covers our use case, we can just set that flag to false in config.json. And with a default to true, it doesn't impact other projects.

Would that be a better PR to do ?

@langleyd
Copy link
Contributor

Hi @estellecomment , Sorry about the late response.

We have since added a config option to remove location sharing from the UI. It's documented here.

UIFeature.locationSharing - Whether or not location sharing menus will be shown.

Thanks for taking the time to contribute.

@langleyd langleyd closed this Mar 28, 2024
@t3chguy
Copy link
Member

t3chguy commented Mar 28, 2024

@langleyd I don't think that changes anything, the proposal here is if there's no map server configured and you'd get an error if you click the location button then you probably shouldn't get a location button. Without requiring deployments to additionally also manually disable location sharing.

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 Z-Community-PR Issue is solved by a community member's PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants