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

Color the trusted domain to alert the admin a bit more #1964

Merged
merged 1 commit into from
Nov 2, 2016

Conversation

nickvergessen
Copy link
Member

Signed-off-by: Joas Schilling <coding@schilljs.com>
@mention-bot
Copy link

@nickvergessen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @jancborchardt and @tanghus to be potential reviewers.

@schiessle
Copy link
Member

schiessle commented Nov 1, 2016

Hm, the crucial question for me is, whether we really want to have a confirm dialog for every trusted server the admin adds? I tend to say no.

Cc @jancborchardt

@LukasReschke
Copy link
Member

LukasReschke commented Nov 1, 2016

Hm, the crucial question for me is, whether we really want to have a confirm dialog for every trusted server the admin adds? I tend to say no.

This is for trusted domains for the trusted domain check. Nothing to do with federated sharing or whatsoever. There it is required. Because you basically click the "Add trusted domain" button and get redirected to this. (i.e. you type nothing)

Also the alert box isn't anything new. It's simply the styling.

@schiessle
Copy link
Member

Ah, OK.... mixed this up. Then it makes sense. (Beside the stupid "!" at the front of the dialog, but I assume that this is a general issue and out of scope for this PR) 👍

@rullzer
Copy link
Member

rullzer commented Nov 2, 2016

LGTM

@rullzer rullzer merged commit 42b0a0d into master Nov 2, 2016
@rullzer rullzer deleted the color-the-trusted-domain-to-alert-the-admin branch November 2, 2016 09:06
@jancborchardt
Copy link
Member

Btw @nickvergessen can we have the button be more contextually relevant here?

Yes shoud be: »Yes, add trusted domain«x

@nickvergessen
Copy link
Member Author

No, the JS thing only has options yes-no and ok

@jancborchardt
Copy link
Member

@nickvergessen well, then it’s of course a request that we should fix it. Opened an issue at #2104 ;)

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

Successfully merging this pull request may close these issues.

6 participants