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

Fixes #3212 : Allow theming for Login/Registration/ResetPassword #3218

Merged
merged 2 commits into from Feb 27, 2019

Conversation

Projects
None yet
3 participants
@TFleury
Copy link
Contributor

TFleury commented Feb 20, 2019

Use admin theme as fallback only

@sebastienros

This comment has been minimized.

Copy link
Member

sebastienros commented Feb 20, 2019

I would prefer to keep the default behavior on the admin side of the things, and only opt-in for it to be served on the front-end, using a custom feature, or a setting. We need to check that it still works fine if no customization is provided in the front-end theme when this feature is enabled.

@TFleury

This comment has been minimized.

Copy link
Contributor Author

TFleury commented Feb 20, 2019

It also fixes #2612 partially.

Having AdminAttribute on those controllers, that are not intended to run in the admin UI has undesired side effects (401 response code).

IMO, serving those page from font-end is the default usage. Serving from admin could be opt-in setting.
Until #2371, serving from front-end was the accepted/normal behavior (like in O1). This behavior was changed because of a malfunction with themeless scenario.

I tested it by my side without any front-end theme and works fine, but I could miss some scenarios.

@Skrypt

This comment has been minimized.

Copy link
Contributor

Skrypt commented Feb 20, 2019

It's been changed for supporting SaaS scenario usage. Not sure if headless is supported. I think the scenarios would be : managed, decoupled, headless.

@sebastienros

This comment has been minimized.

Copy link
Member

sebastienros commented Feb 20, 2019

I don't think most sites care about customizing the login experience. So if we can prevent users from having to customize it, it's better IMO. Otherwise if you have a custom theme and you go to the login screen and the form is completely off because it was not customized. Also, even you the front-end theme was the default one, you would still need custom views. Which means there is no advantage at all in using the FE theme by default. It _might _ just look ok, if you are lucky. But with admin controllers you are sure it will look ok in all cases.

#2612 is a different problem that can be resolved independently by changing the registration feature.

@TFleury

This comment has been minimized.

Copy link
Contributor Author

TFleury commented Feb 21, 2019

I don't think most sites care about customizing the login experience. So if we can prevent users from having to customize it, it's better IMO. Otherwise if you have a custom theme and you go to the login screen and the form is completely off because it was not customized. Also, even you the front-end theme was the default one, you would still need custom views. Which means there is no advantage at all in using the FE theme by default. It _might _ just look ok, if you are lucky. But with admin controllers you are sure it will look ok in all cases.

I agree with that. So, let's go for a opt-in setting to use front-end theme.
Maybe we can have a setting per controller/feature :

public class LoginSettings
{
    public bool UseSiteTheme { get; set; }
}
public class RegistrationSettings {
    public bool UsersCanRegister { get; set; }
    public bool UsersMustValidateEmail { get; set; }
    public bool UseSiteTheme { get; set; }
}
public class ResetPasswordSettings {
    public bool AllowResetPassword { get; set; }
    public bool UseSiteTheme { get; set; }
}

Or do you think one setting would be enough ?

#2612 is a different problem that can be resolved independently by changing the registration feature.

I still think it's mostly due to AdminFilter. We want admin theme, but we don't want to check AccessAdminPanel permission.

@sebastienros

This comment has been minimized.

Copy link
Member

sebastienros commented Feb 21, 2019

Not sure about #2612. Try without it first.

@TFleury

This comment has been minimized.

Copy link
Contributor Author

TFleury commented Feb 22, 2019

I added opt-in settings.
If "Use site theme" is opted in, it lets SiteThemeSelector do its job, but always provides admin theme as fallback (in case there is no site theme).
Othewise, it selects admin theme with same priority as AdminThemeSelector, avoiding AdminAttribute security checks.

@sebastienros sebastienros merged commit 251a036 into OrchardCMS:dev Feb 27, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.