-
Notifications
You must be signed in to change notification settings - Fork 159
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
Fetch settings values from sdk and use language code #3484
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
please update owncloud-sdk and add some error handling |
@@ -0,0 +1,7 @@ | |||
Change: Make settings available in phoenix |
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.
"Enhancement" might be better 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.
Nope, it's a change. ;-) Introduces settings to phoenix => entirely new feature.
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.
Exactly, a new feature which doesn't change any existing functionality
@@ -29,6 +29,7 @@ const actions = { | |||
logout(context) { | |||
const logoutFinalizer = () => { | |||
context.dispatch('cleanUpLoginState') | |||
context.dispatch('loadSettingsValues') |
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.
Why load the settings during logout?
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.
Because there will be settings that exist without user context. For example, when we provide multiple themes in the future, an admin could set a default theme in settings. That has to be available even if you don't have a user context. So as a result, all changes to the authenticated user (changing to another one or signing out) need to trigger a settings reload.
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 see. I am just a bit afraid of relying on authentication in this case because of the public link's context.
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.
Looks great! Settings is amazing progress 🚀
This PR depends on owncloud/owncloud-sdk#475, so this has to stay WIP until the SDK-PR is merged and released.
Description
We have a settings service, that is supposed to be integrated in Phoenix like described in the docs
This PR uses settings values, coming from the settings service, to adapt the Phoenix UI for the authenticated user. At the moment, only the
language
setting is used: Changing the language in the settings frontend (has to be configured as a Phoenix app) results in having another language in Phoenix. In the future we can make use of this for a variety of customization:Related Issue
owncloud/product#17
Motivation and Context
We want to provide a customized user experience for users.
How Has This Been Tested?
So far only in Chrome and Firefox. No tests implemented so far.
Screenshots (if appropriate):
English UI
Changed language to French
French UI (at least everything that's translated already)
Types of changes
Checklist:
Open tasks: