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

Add tab if alternate login present #16832

Merged
merged 12 commits into from
Sep 11, 2019
Merged

Add tab if alternate login present #16832

merged 12 commits into from
Sep 11, 2019

Conversation

ochorocho
Copy link
Contributor

@ochorocho ochorocho commented Aug 21, 2019

Recently installed Nextcloud for roughly 40 users. Users are supposed to login using Gitlab login provided by sociallogin app.

Not even half of the users noticed the "Alternate Login" button. Therefore i created a tab menu for login. I also introduced a setting in admin area to force alternate login as "active" tab.

Bildschirmfoto 2019-08-22 um 01 04 16

When no alternate login is available:

Bildschirmfoto 2019-08-24 um 19 47 40

@rullzer
Copy link
Member

rullzer commented Aug 22, 2019

Could you rebase your PR ;)
As 12010 files changes seems a tad much 😉

@ochorocho
Copy link
Contributor Author

Could you rebase your PR ;)
As 12010 files changes seems a tad much 😉

Tbh, was late yesterday and i screwed it up while signing my commits...

@rullzer do you like the idea?

You can test it here: https://nextcloud.knallimall.org/

@rullzer
Copy link
Member

rullzer commented Aug 22, 2019

I'm fine but I'd like the JS experts and designers to have a look ;)

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan. I think we should improve the visibility of the alternative logins instead of adding a new visual element (which is not used anywhere else in nextcloud). For example the stackoverflows login screen looks very clean.
image

core/Controller/LoginController.php Outdated Show resolved Hide resolved
core/Controller/LoginController.php Outdated Show resolved Hide resolved
settings/templates/settings/admin/server.php Outdated Show resolved Hide resolved
settings/Settings/Admin/Server.php Outdated Show resolved Hide resolved
core/templates/login.php Outdated Show resolved Hide resolved
@splitt3r
Copy link
Contributor

Off topic: @ochorocho Awesome that you contribute to GitLab and now Nextcloud 😍 Great work!

@jancborchardt
Copy link
Member

Good initiative @ochorocho!

Agree with @kesselb here that instead of making the design more complex, the other logins should be made more present by being moved up like on other platforms. :)

Then not even an extra admin setting to set the active tab is necessary because there are no tabs. Easier for everyone. :)

@ochorocho
Copy link
Contributor Author

Ok :-) Still like the tab view thing.
Created a little theme which does what i need, see https://github.com/ochorocho/nextcloud-theme-tablogin

Works with NC 16 and NC 17 (current master)

@jancborchardt
Copy link
Member

@ochorocho @kesselb would someone of you be up to change it as @kesselb described? Would certainly be good to fix this in core and not just as a theme. :)

@ochorocho ochorocho closed this Aug 26, 2019
@kesselb
Copy link
Contributor

kesselb commented Aug 26, 2019

Adding a logo and another color seems to be more complicated. I think this information should be provided by https://github.com/zorn-v/nextcloud-social-login and passed as additional data (e.g. a logo) to

public static function registerLogIn(array $entry) {
self::$altLogin[] = $entry;
}

As a start we should set a custom class to <li> based on the name or href to apply custom styles more easily.

<?php foreach($_['alt_login'] as $login): ?>
<li><a class="button" href="<?php print_unescaped($login['href']); ?>" ><?php p($login['name']); ?></a></li>
<?php endforeach; ?>

Signed-off-by: ochorocho <rothjochen@gmail.com>
@ochorocho ochorocho reopened this Aug 26, 2019
@ochorocho
Copy link
Contributor Author

@jancborchardt just moved the buttons above the form and optimized alignment of the buttons

See https://nextcloud.knallimall.org/

@kesselb
Copy link
Contributor

kesselb commented Aug 26, 2019

image

I would suggest to add "log in with" to make the purpose of the button more clear. <?php p($l->t('Log in with')) ?>.

@kesselb
Copy link
Contributor

kesselb commented Aug 26, 2019

https://github.com/zorn-v/nextcloud-social-login/blob/master/css/style.css Hmm. There is already a style sheet with logos for github.

@ochorocho
Copy link
Contributor Author

Yes, only for the pre-defined oauth provider but not for "OpenID Connect/Custom OpenID Connect"

Bildschirmfoto 2019-08-26 um 20 42 59

@jancborchardt
Copy link
Member

Great stuff @ochorocho!
Agree with adding the "Log in with …" before the provider that @kesselb mentioned. Also, right now it’s a bit confusing cause there are 3 primary buttons:

  • The buttons should be colored according to the provider. Google red, Twitter light blue, Facebook dark blue, GitLab dark purple, GitHub dark grey etc.
  • Also I would add icons (monochrome, that is only white) to the left inside the icons for better recognition, as this is also standard for "Log in with" buttons. We have some of these icons already in https://github.com/nextcloud/server/tree/master/core/img (like twitter.svg)
  • There should be a tiny bit more whitespace between the log in buttons and the form for username and password, just for visual separation.

Then I think it would be super.

@ochorocho
Copy link
Contributor Author

ochorocho commented Aug 27, 2019

@jancborchardt @kesselb this is what i'm going to do.

Todo Nextcloud

  • More space between login buttons and form

Todo in Social login

  • Color Buttons
  • Add predefined icons (monochrom)
  • Add "Login in with"

@jancborchardt
Copy link
Member

Yup, sounds great @ochorocho! :) Thanks a lot.

And maybe with the visual changes to the buttons it’s also fine to put them below the "Log in" button again. So like:

[       Username      ]
[       Password      ]
(       Log in →      )
          or
( Log in with Google  )
( Log in with GitHub  )

Signed-off-by: Jochen <rothjochen@gmail.com>
Signed-off-by: Jochen <rothjochen@gmail.com>
@ochorocho
Copy link
Contributor Author

This is how it will like now...
Still need open a pr for the sociallogin repo.

Screenshot_2019-09-03 Nextcloud(1)
Screenshot_2019-09-03 Nextcloud

@skjnldsv
Copy link
Member

skjnldsv commented Sep 4, 2019

I guess no one will have that many, right? ^^'

@ochorocho
Copy link
Contributor Author

@zorn-v Yep, another idea would be to remove "Log in with" on small viewports.

@zorn-v
Copy link
Member

zorn-v commented Sep 6, 2019

I think it is good compromise

@zorn-v
Copy link
Member

zorn-v commented Sep 6, 2019

What color would you suggest as "not blue" ? :-)

I think someting like border: 0 or border-color for colored buttons. Blue buttons for alternative logins without borders may be unnoticeable I think.

@jancborchardt
Copy link
Member

What color would you suggest as "not blue" ? :-)

Simply the class="primary" needs to be removed from the button, then it should work. :)

Signed-off-by: Jochen <rothjochen@gmail.com>
@ochorocho
Copy link
Contributor Author

@jancborchardt @zorn-v

  • removed class primary , after that registration buttone was white background with white text, so i changed the text-color to blue.
  • Removed "Log in with" on small viewports (< 1024px)
  • min-width was set to align width to "Log in"-Button, if more space needed it will take up to 400px

responsiveness

Screenshot_2019-09-06 Nextcloud

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super impressive work @ochorocho! Looks perfect now 🚀

Please review too @nextcloud/designers :)

@jancborchardt
Copy link
Member

Oh just saw the:

Removed "Log in with" on small viewports (< 1024px)

I would say this makes it confusing. I’d rather say cut the icon on mobile, or ellipsize. But the label needs to be explanatory. Otherwise it’s like "Google? What, is that a link to the service?"

(also small typo: It’s "Salesforce" not "Saleforce", not sure in which component that is. :)

#16832 (comment)

Signed-off-by: Jochen <rothjochen@gmail.com>
@ochorocho
Copy link
Contributor Author

ochorocho commented Sep 7, 2019

@jancborchardt only icons hidden on small devices now...

hide_icons_only

@kesselb
Copy link
Contributor

kesselb commented Sep 8, 2019

Thank you @ochorocho for this great contribution 🎉 and also thanks to @zorn-v for your feedback over here 👍

core/templates/login.php Outdated Show resolved Hide resolved
core/templates/login.php Outdated Show resolved Hide resolved
core/templates/login.php Outdated Show resolved Hide resolved
Signed-off-by: Jochen <rothjochen@gmail.com>
@ochorocho
Copy link
Contributor Author

I thinks this is ready now :-)

core/templates/login.php Outdated Show resolved Hide resolved
#16832 (comment)

Signed-off-by: Jochen <rothjochen@gmail.com>
#16832 (comment)

Signed-off-by: Jochen <rothjochen@gmail.com>
#16832 (comment)

Signed-off-by: Jochen <rothjochen@gmail.com>
.wrapper {
width: 300px;
max-width: 100%;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could lead to issues in other layouts, but we'll have a look later I guess, since we're at the start of 18!

@skjnldsv
Copy link
Member

A few unrelated failures :)
Merging!

@skjnldsv skjnldsv merged commit 0cc780e into nextcloud:master Sep 11, 2019
@welcome
Copy link

welcome bot commented Sep 11, 2019

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!

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.

None yet

7 participants