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

Improve login screen #38506

Closed
wants to merge 50 commits into from
Closed

Improve login screen #38506

wants to merge 50 commits into from

Conversation

hurradieweltgehtunter
Copy link
Contributor

@hurradieweltgehtunter hurradieweltgehtunter commented Mar 12, 2021

Description

When the new OIDC / AzureAD Login was presented the old-fashioned grey "Login with AzureAD" button did not represent this modern feature accordingly. Also the missing dedicated Login-Button was bugging me for years.
This PR adds some styling, makes the HTML valid and adds a dedicated login button.

How Has This Been Tested?

Latest Firefox, Latest Chrome, Latest Safari

Screenshots (if appropriate):

Before:
grafik

After:
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

QA

  • (1) Deleting the background shows weird behaviour
  • (2) Setup screen needs some adjustments
  • (3) When setting a password in setup screen the password indicator is inside the password field

@update-docs
Copy link

update-docs bot commented Mar 12, 2021

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.

@hurradieweltgehtunter
Copy link
Contributor Author

One thing to keep in mind (mentioned by @ChrisEdS ): For custom brandings the button color should be included in the styling process.

@micbar
Copy link
Contributor

micbar commented Mar 15, 2021

@pmaier1 @hurradieweltgehtunter Ok for me to keep the arrow and add the login button.

@phil-davis but we need to make sure that we really press the login buttton in the CI

@hurradieweltgehtunter
Copy link
Contributor Author

hurradieweltgehtunter commented Mar 15, 2021

There's multiple possibilities:

  1. Keep it as it is now (keep the arrow) -> no functional changes, tests should pass; Okayish UX
  2. Remove arrow & put loading animation in the login button -> tests need to be adapted if they target the arrow submit button in specific; Best UX

I'd prefer 2) UX wise. Let me know if I should do this change also.

@AlexAndBear
Copy link

AlexAndBear commented Apr 14, 2021

Beautiful 1 !

@hurradieweltgehtunter encountered a little bug with IE11:
clicking in the input field will clear the placeholder

image

@AlexAndBear
Copy link

AlexAndBear commented Apr 14, 2021

One more thing: hovering over the -> login arrow button, will now make the item disappear
(Only in Chrome, IE11 works)

@hurradieweltgehtunter
Copy link
Contributor Author

@ChrisEdS yould you re-check with errors end customer themes?

@hurradieweltgehtunter
Copy link
Contributor Author

Changes are final, ready to merge.
owncloud/guests#446 needs to be merged too

@ChrisEdS
Copy link
Contributor

@hurradieweltgehtunter Please document how the users/customers can theme the new elements. That should be part of the admin release notes as well. // cc @pmaier1

@cdamken

This comment has been minimized.

@AlexAndBear
Copy link

AlexAndBear commented May 20, 2021

@cdamken old version ? invalidate your cache please
@hurradieweltgehtunter I will update the before/after images in the entry description

@sonarcloud
Copy link

sonarcloud bot commented May 21, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM. There are 50 commits here, and the rebase process was giving me conflicts...

I squashed all the commits into another branch on top of current master - see PR #38821 - we could actually merge the code from that PR with a single clean commit. The changelog link could be left as it is, pointing to this PR, because this PR is where all the discussion and history is.

@phil-davis phil-davis mentioned this pull request Jun 9, 2021
11 tasks
hurradieweltgehtunter added a commit to owncloud/guests that referenced this pull request Aug 3, 2021
hurradieweltgehtunter added a commit to owncloud/guests that referenced this pull request Aug 4, 2021
phil-davis pushed a commit to owncloud/guests that referenced this pull request Sep 1, 2021
phil-davis pushed a commit to owncloud/guests that referenced this pull request Sep 1, 2021
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.

9 participants