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

Refactoring the login page to allow template usage #4964

Merged
merged 6 commits into from
Jul 6, 2020
Merged

Refactoring the login page to allow template usage #4964

merged 6 commits into from
Jul 6, 2020

Conversation

akiyamaSM
Copy link
Contributor

Allow to use the login template in other pages such as Reset password if needed

@extends('voyager::auth.master', ['title' => 'Reset Password'])

@section('content')

@endsection

@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #4964 into 1.4 will not change coverage.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##                1.4    #4964   +/-   ##
=========================================
  Coverage     62.96%   62.96%           
  Complexity     1372     1372           
=========================================
  Files           194      194           
  Lines          4007     4007           
=========================================
  Hits           2523     2523           
  Misses         1484     1484           
Impacted Files Coverage Δ Complexity Δ
src/Traits/Translatable.php 37.65% <75.00%> (ø) 61.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d484e8...0534d72. Read the comment docs.

@akiyamaSM
Copy link
Contributor Author

Hello @emptynick Hope it doesn't bother you taking look at this PR

@emptynick
Copy link
Collaborator

This would be a breaking change.
Why not simply add @section('...') and @show around the login-container in login.blade.php to make it overridable?

@akiyamaSM
Copy link
Contributor Author

Why is a breaking change?
And in my case i have pages where i need to reset the password and I have to change in the title too.

@emptynick
Copy link
Collaborator

Because other people might have overriden this view as well.
The title could also be just a section/yield

@akiyamaSM
Copy link
Contributor Author

akiyamaSM commented Jun 2, 2020

I really think it should be out of the box. But since its a breaking change. Hope it can make to an other major version.

@emptynick
Copy link
Collaborator

But like I said before, it can be done much more easy with ~3 lines of code.

@MrCrayon
Copy link
Collaborator

MrCrayon commented Jun 2, 2020

As @emptynick said title could be:

@yield('title', 'Admin - '.Voyager::setting("admin.title"))

That way we can keep compatibility and it's easily overridable.

@akiyamaSM
Copy link
Contributor Author

I really don't see why it shouldn't be part of its core. I believe that reseting password's flow should be a part of the package.

@MrCrayon
Copy link
Collaborator

MrCrayon commented Jun 2, 2020

I didn't say I'm against this PR, personally I think it makes sense.

I'm only saying that I like more the use of yield for title.
But I have to correct myself, that's not a breaking change because right now users either keep the template or replace it completely.

@akiyamaSM
Copy link
Contributor Author

@MrCrayon thanks, Ill make the change about the yield to fit the requirements

@akiyamaSM
Copy link
Contributor Author

Hello @MrCrayon Changes has been made :D

@akiyamaSM
Copy link
Contributor Author

Any ups ?
@MrCrayon
@emptynick

Copy link
Collaborator

@MrCrayon MrCrayon 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 good with this, now you have to convince @emptynick 😁

@akiyamaSM
Copy link
Contributor Author

@MrCrayon hahaha thanks.

@akiyamaSM
Copy link
Contributor Author

Hey @emptynick Hope we can get an answer, I have an other PR that I want to submit for a fix.

@emptynick
Copy link
Collaborator

Always create a sub-branch from 1.4 so you can create multiple PRs at the same time.

@emptynick emptynick merged commit 294ff76 into thedevdojo:1.4 Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants