-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
config parameter disabling forcing sso iframe #463
config parameter disabling forcing sso iframe #463
Conversation
would fix #462 |
005b24b
to
5d62585
Compare
@gary-kim it should build, I only changed one file and it complains about missing package:
could you have a look at it? |
Now it passes all checks, have only changed |
Yeah, my bad. I think the build fixed itself because I updated one of the container images used in the build process. |
So, to add a new config, you will need to add it here with the default value: riotchat/lib/AppInfo/Application.php Line 38 in 5b10eed
then make it available to the frontend where you want to use it by adding another one of these: riotchat/lib/Controller/AppController.php Line 60 in 5b10eed
then add it to the admin settings by putting it here (should be similar to the disable custom URLs setting): riotchat/src/components/AdminSettings.vue Lines 57 to 67 in 5b10eed
and here riotchat/src/components/AdminSettings.vue Line 234 in 5b10eed
|
Have done it
I am not sure if I implemented this part correctly. |
Maybe need documentary somewhere, if someone is using custom config. |
7704807
to
997c579
Compare
@gary-kim did you check if implemented this part correctly? Also where would be a good place to document this? |
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.
Okay, tested it myself. It does work though the text of the setting and the effect of the setting seem reversed at the moment.
I think we can just fix that and make the setting name a bit clearer and that should be done!
As far as documentation goes, I think just being in the settings will be good enough for now. If questions start being asked, we can make some system for documentation. For now I'd prefer to have it be self-documenting if possible.
src/main.js
Outdated
window.localStorage.setItem.apply(this, params); | ||
}; | ||
// Setting sso_force_iframe (in config) to true forces iframe even if using SSO or CAS login | ||
if (loadState('riotchat', 'sso_force_iframe') !== 'false') { |
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.
if (loadState('riotchat', 'sso_force_iframe') !== 'false') { | |
if (loadState('riotchat', 'sso_force_iframe') === 'false') { |
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 more setting it to !== 'true'
would be better, if the value is anything else it will then be treated as false
, but your suggestion is easier to read.
Signed-off-by: jonathanmmm <[email protected]>
Signed-off-by: jonathanmmm <[email protected]>
Signed-off-by: jonathanmmm <[email protected]>
Update src/components/AdminSettings.vue Co-authored-by: Gary Kim <[email protected]> Signed-off-by: jonathanmmm <[email protected]>
Signed-off-by: jonathanmmm <[email protected]> Signed-off-by: jonathanmmm <[email protected]>
Co-authored-by: Gary Kim <[email protected]> Signed-off-by: jonathanmmm <[email protected]>
204e6d6
to
8d14841
Compare
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.
Alright, tested and it seems to work great! Thank you!
haven't checked if it works