-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(web): add privacy step in the onboarding #11359
Conversation
3ffe6aa
to
4ec1cf9
Compare
Crazy fast! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noice!
web/src/lib/components/onboarding-page/onboarding-privacy.svelte
Outdated
Show resolved
Hide resolved
Wow ! Thanks for handling this so quickly ! Can we consider cherry-picking commits e1f08a5 and af0f53d from my PR into this branch ? e1f08a makes the settings page clearer, and af0f53d rewords the guide in the docs so its easier to add guides for more alternative tile providers. This leaves the Nginx tutorial I proposed out of this PR. I'm also wondering why one of the two switches mentions requests to Github, but the other one does not mention Cofractal as the default map provider. (Edit : If you merge e1f08a5 into this branch, the onboarding screen could include the Last thing (but this may be out of scope for this PR) is that since this is a server-side toggle, the F-Droid build will stil require either a client side opt-in toggle with clear details on the implications, or a "Non-free network services" tag |
In only cherry-picked e1f08a5, it makes sense to keep your PR for the documentation.
Done with 4678dc7
Yes, that's out of scope of this PR |
@pcouy Btw, what do you think about renaming |
This seems fine to me. While we're rephrasing this translation key, we should probably add a mention to the default tile provider. Something like |
I went ahead and rebased the branch to keep the commit history as readable as possible :
The rebased branch is available at https://github.com/pcouy/immich/commits/feat/onboarding-privacy/ |
I like having a clean git history, but we squash commits so it doesn't really matter.
I'll add it this evening |
You can still use the rebased branch, or just patch-in the output of
|
Oh thanks, you're making it really easy 😄 |
Should the |
IMO, that's a small change and it's absolutely fine to have it in one of our PRs, preferably yours, so we can discuss about the wording (if we need to) there. Nothing has been merged yet, so let's keep it simple. |
web/src/lib/i18n/en.json
Outdated
"map_light_style": "Light style", | ||
"map_manage_reverse_geocoding_settings": "Manage <link>Reverse Geocoding</link> settings", | ||
"map_reverse_geocoding": "Reverse Geocoding", | ||
"map_reverse_geocoding_enable_description": "Enable reverse geocoding", | ||
"map_reverse_geocoding_settings": "Reverse Geocoding Settings", | ||
"map_settings": "Map Settings", | ||
"map_settings_description": "Manage map settings", | ||
"map_settings_description": "Manage map settings. <link>Alternative tile providers can be chosen by changing the style.json map theme below.</link>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these links go to the same place. I'm also not a huge fan of having the entire row be a clickable button, and then also having a link embedded it in . Can we move this note and associated link to the top of the "Map & GPS Settings" section instead? (no more links in accordion descriptions).
Something like:
For more information about these settings, refer to the documentation.
That could be something like admin.refer_to_docs
and we could link it to reverse geocoding in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed them all and links can be added on the web once the explanations are added in the doc.
web/src/lib/i18n/en.json
Outdated
@@ -127,13 +127,14 @@ | |||
"map_enable_description": "Enable map features", | |||
"map_gps_settings": "Map & GPS Settings", | |||
"map_gps_settings_description": "Manage Map & GPS (Reverse Geocoding) Settings", | |||
"map_implications": "Enabling the map implies clients (web and mobile apps) sending requests to a third-party map server in order to display the map. These requests include the client's IP address, the Immich instance URL, as well as the coordinates and zoom level of the requested map tiles. The default tile provider is tiles.immich.cloud", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to be so detailed here. The settings page don't need to be 100% self documenting. A general statement is fine and more details can live at immich.app. I would prefer to see something like:
The map feature relies on external service to function properly. Read more about those implications here
web/src/lib/i18n/en.json
Outdated
@@ -841,6 +842,7 @@ | |||
"ok": "Ok", | |||
"oldest_first": "Oldest first", | |||
"onboarding": "Onboarding", | |||
"onboarding_privacy_description": "To work properly, Immich needs to rely on external services such as a tile provider used for the map feature and Github to be aware of new releases", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a bad idea to list specific features in this translation. That just means it will be wrong whenever we add another service. What about something like:
The following features rely on external services to function properly:
If we add another service, we can just refer to new keys/translations in the card. We can also change/remove services as needed without requiring existing translations to be updated.
Added a link to the guide on customizing map styles as well
d8db3db
to
9917cc0
Compare
9917cc0
to
e694580
Compare
* feat: add privacy step in the onboarding * fix: remove console.log * feat:Details the implications of enabling the map on the settings page Added a link to the guide on customizing map styles as well * feat: add map implication * refactor: onboarding style * fix: tile provider * fix: remove long explanations * chore: cleanup --------- Co-authored-by: pcouy <[email protected]> Co-authored-by: Jason Rasmussen <[email protected]>
Following the dicussion in #11350, add a new step in the onboarding to make clear which external services are used by Immich and an option to disable each service.
Screenshots