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

V8: UI — updated login screen #4754

Merged
merged 7 commits into from Mar 11, 2019

Conversation

@nielslyngsoe
Copy link
Contributor

commented Feb 26, 2019

Updated look of login screen

@bjarnef

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@nielslyngsoe it seems this change removes the option to set a custom background image on login screen introduced introduced in Umbraco v7.6

We use this for some of our clients and it feels a bit more as their own, while still keeping the Umbraco branding.
It is configurable from this config setting:
https://github.com/umbraco/Umbraco-CMS/blob/dev-v8/src/Umbraco.Web.UI/config/umbracoSettings.config#L57

@nielslyngsoe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@bjarnef I will look into that. It makes good sense, and I would love clients to feel its their own :-)

@nielslyngsoe nielslyngsoe removed their assignment Mar 7, 2019

@nielslyngsoe nielslyngsoe requested review from nul800sebastiaan and removed request for nul800sebastiaan and RobertCopilau Mar 7, 2019

@nielslyngsoe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

now configurable again! :-)

correcting path in umbracoSettings.Release.config
+ changed comment to fit with new look

@ghost ghost assigned nielslyngsoe Mar 11, 2019

@bergmania bergmania merged commit 8f3bcf8 into dev-v8 Mar 11, 2019

1 check was pending

Cms 8 Continuous in progress
Details

@ghost ghost removed the state/review label Mar 11, 2019

@bergmania bergmania deleted the temp8-ui-update-login-screen branch Mar 11, 2019

@nielslyngsoe nielslyngsoe removed their assignment Mar 12, 2019

@ronaldbarendse

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

Why is background-image: url('../img/login.jpg'); added in the CSS and the ng-if="vm.backgroundImage" removed?

This way, the overlay div is always rendered (even when the loginBackgroundImage is removed/set to empty) and will contain an invalid URL, so it also won't show the added background-image anyway...

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Is there an actual problem? Both Chrome and FF seem to handle an empty URL just fine. Nothing erroring in the network tab either.

image

@ronaldbarendse
Copy link
Contributor

left a comment

But it has an invalid background-image (url((unknown))) and doesn't show the fallback image (../img/login.jpg) either.

}

.login-overlay__background-image {
background-position: center center;
background-repeat: no-repeat;
background-size: cover;
background-image: url('../img/login.jpg');

This comment has been minimized.

Copy link
@ronaldbarendse

ronaldbarendse Jul 23, 2019

Contributor

The background image should only be set in the Umbraco settings.

Suggested change
background-image: url('../img/login.jpg');
@@ -2,10 +2,10 @@

<div id="login" class="umb-modalcolumn umb-dialog" ng-class="{'show-validation': vm.loginForm.$invalid}" ng-cloak>

<div class="login-overlay__background-image" ng-if="vm.backgroundImage" ng-style="{'background-image':'url(' + vm.backgroundImage + ')'}"></div>
<div class="login-overlay__background-image" ng-style="{'background-image': 'url('+vm.backgroundImage+')'}"></div>

This comment has been minimized.

Copy link
@ronaldbarendse

ronaldbarendse Jul 23, 2019

Contributor

If no background image is set (in the Umbraco settings), we should not render the overlay div (this also prevents an empty/invalid background-image to be set).

Suggested change
<div class="login-overlay__background-image" ng-style="{'background-image': 'url('+vm.backgroundImage+')'}"></div>
<div class="login-overlay__background-image" ng-if="vm.backgroundImage" ng-style="{'background-image': 'url('+vm.backgroundImage+')'}"></div>
@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

It's just empty.. and that's.. fine?

image

Why would we force a background image if you clearly didn't want one? :-)

@ronaldbarendse

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

Well not quite, as an empty value could cause the current page to be requested again:

And exactly like you say, why force a background-image if you don't want one, but then include a fallback in the default styles?

background-image: url('../img/login.jpg');

@bjarnef

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

I guess the background image just can be set from the controller and remove the hardcoded css background image.

vm.backgroundImage = Umbraco.Sys.ServerVariables.umbracoSettings.loginBackgroundImage;

and then only render the overlay if vm.backgroundImage has a value.

Maybe with some basic validation or the path to the login background image is valid before assigning the value to vm.backgroundImage.

@ronaldbarendse

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

@bjarnef Just like the suggested changes I already added to this PR earlier 😉 I would keep the basic validation to empty/set and not add any other checks, as some might also want to use absolute URLs or data-URIs, etc.

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Gentlemen, I think your feedback is wonderful and I appreciate the engagement but can we please be pragmatic:

  1. Is there an actual practical problem that is not a theoretical "could cause the current page to be requested again"
  2. Should we be spending our energy on more important things than the possibility of someone leaving a config value empty?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.