rappasoft / laravel-boilerplate

The Laravel Boilerplate Project - https://laravel-boilerplate.com
https://rappasoft.com
5.59k stars 1.58k forks source link

5.3 Update #485

Closed rappasoft closed 8 years ago

rappasoft commented 8 years ago

Been working on a ground up since 5.3 came out. Much more changed than I thought. Probably about 30% done.

Will keep everyone posted.

digitalit commented 8 years ago

Keep up the great work. It is much appreciated @rappasoft :)

ghost commented 8 years ago

I've been transferred to version 5.3. Later we can compare.

rappasoft commented 8 years ago

I've started with a fresh 5.3 install. It is very much different so far. It wasn't as easy as dragging all of the folders over unfortunately.

pebbo commented 8 years ago

Currently working on adding translatable to the roles and permissions... do you think I should wait until the 5.3 boilerplate update is ready?

rappasoft commented 8 years ago

It's all coming together nicely, complete with new Laravel features:

screen shot 2016-09-23 at 11 44 41
poseso commented 8 years ago

nice!

chemitaxis commented 8 years ago

Thank you so much for your work...

LunarDevelopment commented 8 years ago

Dare you give an ETA?

Amazing work so far by the way, Thankyou very much.

On Sun, 25 Sep 2016, 10:06 a.m. Chema, notifications@github.com wrote:

Thank you so much for your work...

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rappasoft/laravel-5-boilerplate/issues/485#issuecomment-249411079, or mute the thread https://github.com/notifications/unsubscribe-auth/AKNqM6uhJsGNo5Q7POBD47hiMAf3JBGJks5qtjmFgaJpZM4KCBVs .

rappasoft commented 8 years ago

Have a feeling this one is going to be V4 because it's changed way too much for me to not justify it.

Hoping a week or two max.

blomdahldaniel commented 8 years ago

Thats so awesome!!

poseso commented 8 years ago

that's great!

digitalit commented 8 years ago

Anyone knows how this would work upgrade-wise: laravelshift.com

rappasoft commented 8 years ago

Got stuck on the stupidest of problems, back on track just lost a few days.

rappasoft commented 8 years ago

Since Vue 2.0 came out, I have to figure out what I need to change upgrade now. Bad timing.

blomdahldaniel commented 8 years ago

Haha! :) That timing! Looking forward to see your work!

rappasoft commented 8 years ago

Vue 2.0 working. BTW i'll get to the issues and PR's within this release. So when it launches i don't have to merge anything and will probably close it all.

It's getting there. Haven't had time the last couple days. But I dramatically simplified things.

I'll put it on development when i'm satisfied, but will need more eyes before I release.

minedun6 commented 8 years ago

is it possible to make something so that we can understand how to integrate another theme ? let's say metronic theme maybe or any other bootstrap theme ^^. Something else, sometimes when i submit a form, it logs me out even when I am on admin. The login area doesn't show the error message sometimes ^^. Last thing is that this package needs some serious documentation since it's getting more and more attention.

Keep it up my friend.

rappasoft commented 8 years ago

If anyone wants to try what I have so far it is on development: https://github.com/rappasoft/laravel-5-boilerplate/tree/development

Right now everything works, but I've yet to refactor some things and do all of the issues and pull requests.

activemonkeys commented 8 years ago

Thank you Anthony. Just got it up and running and was able to login (after db:seed) with admin@admin.com. So far so good.

rappasoft commented 8 years ago

Cool let me know what you think.

poseso commented 8 years ago

I just tried it, everything is working smoothly, just seen a minor typo when permantely delete a user in the sweet alert, but everything else is working correctly

brentarcane commented 8 years ago

Looks good! Just tinkering with the access management - when I disable a user, I can't re-enable them. They simple don't appear on the list almost as if they have been deleted.

rappasoft commented 8 years ago

They're not in the deactivated section?

On Oct 10, 2016, at 06:54, Brent Kozjak notifications@github.com wrote:

Looks good! Just tinkering with the access management - when I disable a user, I can't re-enable them. They simple don't appear on the list almost as if they have been deleted.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

brentarcane commented 8 years ago

My bad, didn't notice that section! Works fine, please ignore.

rappasoft commented 8 years ago

I'll modify where it routes you depending on selection to avoid confusion.

On Oct 10, 2016, at 08:23, Brent Kozjak notifications@github.com wrote:

My bad, didn't notice that section! Works fine, please ignore.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

activemonkeys commented 8 years ago

Just one minor thing I came across so far

Log Viewer - viewing the log data for a certain date - when pressing "All" -> InvalidArgumentException in compiled.php line 9541: Route [log-viewer::logs.show] not defined.

Furthermore it seems to be working smoothly. Within the coming days I'll start a new test project. Will let you know when I come across other things.

As for the AdminLTE theme. In the demo on https://almsaeedstudio.com/ you'll see that all menu items have an icon in front of them and these icons remain visible when the sidebar is closed. No problem to implement this functionality myself. However, it would be nice when it's part of your the boiler plate.

Thanks for all your work so far. This really is a time saver. Especially the clear split between frontend/backend is much appreciated!

daniel-farina commented 8 years ago

Keep up the great work, this is the bet way to get started with Laravel, period.

Dan

rappasoft commented 8 years ago

@activemonkeys Good idea I never even noticed, I added the icons and the mini-sidebar feature. Still trying to figure out the log viewer thing.

activemonkeys commented 8 years ago

Thanks for implementing the icons and mini-sidebar feature Anthony! Much appreciated.

Ran into another minor issue:

app\Http\Middleware\RedirectIfAuthenticated.php

it says:

if (Auth::guard($guard)->check()) {
        return redirect('/home');
 }

However /home is an undefined route

num3thod commented 8 years ago

@activemonkeys So in other words, your test app is...homeless?

Good catch. Should be ('/') I guess. Or whatever your specific app requires.

activemonkeys commented 8 years ago

@num3thod yes, my app is looking for shelter! Changed it also to ('/'). For other users however, it would be nice when it's corrected in the boiler plate.

Also I noticed when the (new) Dutch language is selected the word "Language" remains visible in the selector. The Dutch word for "Language" is "Taal". Would be nice when this is corrected too, should there be any Dutch users..

num3thod commented 8 years ago

Agree completely

rappasoft commented 8 years ago

I took the dutch pack from the open pull requests, I don't speak it, but I didn't look through the translations. Looking through it now there's many lines that were not translated from english.

Good catch on the middleware route.

blomdahldaniel commented 8 years ago

This seems great!

Suggestions, (i can fix it myself but want to hear what you guys think).

  1. Remove "Change Password" from frontend/includes/nav.blade.php. It feels neither necessary nor common to have such a link directly from the navbar.
  2. Change name of the "dashboard" to something more like "My profile". So in moth the navbar and in the routes instead use the term profile. In my head, that would feel more easy to understand and more like other applications. I realise that this would have to change in a lot of places, names of files routes but in my head it would be worth it.
  3. The idea of deactivated users. Is that like banned users? Maybe that would be a term that would feel more natural? Because activated and deactivated could be confused with the fact that you confirm your account. And if you haven't confirmed your account the account is unconfirmd = not active..? So therefore the terminology about banned users would make more sense? So that when you create a user from the adminpanel you don't se "Active: checkbox". Instead you may check the box "banned". What do you guys think of that?

Thoughts?

I will go over the Swedish translations as soon as this reaches a more stable stage. :)

rappasoft commented 8 years ago
  1. Yeah I don't know why that's been there since day 1.
  2. I agree, but I also envisioned that gaining more tabs that interacted with the backend app, and it acting as the users dashboard for managing the app, just like the admin section is the dashboard for administrating the app. But I guess Profile has the same meaning. I may be old school but profile used to just be like your name and bio and stuff. Maybe we rename the tab in the dashboard to My Profile?
  3. In my head it's one of those features that's not specific, so the person making the application can decide what they want the active/deactive states to represent. Since you can't log in unless youre active.
blomdahldaniel commented 8 years ago

1.

I thought it was something like that

3.

Okey, then I can see why banned might not be appropriate.. But still, active kind of mess around with the whole idea of confirmed and not confirmed accounts.. Could frozen or something like that be a term that would be easier to understand?

2.

Ah, I see! Hm.. In my head, the profile shouldnt be that nested into the dashboard. The dashboard is the aplications space, not the profile. I think that should be a separate section. See my examples. I understand what you are going for, you are going for tabs, but in my opinion that is maybe not the best way to go. First, a link to "Edit my profile" or "Account settings" should always be in the navbar dropdown-menu. And if we add that, then we would have a link pointing to /dashboard but then have to open the tab "My Profile". I dont think thats the way to go actually. They should be separated.

Here's a good example from Mailgun that explains my thoughts on a dashboard. The application-bound data, in this case the email-graph should be the main section/purpose. The settings for the user should not be there at all. A simple profile overview of the users data could be there but not the entire "My profile". So a summarize, the name maybe profile picture is good but the entire profile and edit edit section should not be there. In this example, Mailgun has a link to Account settings sceenshot-edited

This is the Dashboard for mailchimp, thats the same case there, but in this case I actually think they did it wrong and complicated when they have a link to Profile and Account. And they are very similar. So not so good imo. But still, this is a dashboard that doesnt even directly refer to the "users space". You access it through the dropdown.

skarmavbild 2016-10-12 kl 10 29 44

Github is also a great example, they neither have the direct link to settings/edit profile. They keep it in the dropdown. skarmavbild-2016-10-12-kl -10 36 39

May I create a PR with a refactor of the entire dashboard and edit profile stuff?


EDIT See #499 for my example

rappasoft commented 8 years ago

That's a crazy PR i'll have to sift through it.

Unrelated:

I installed yarn on my machine, I think you should all do the same.

I've just included a yarn.lock file with the project, so now when you install, instead of running npm install, you will run yarn.

As well as adding packages use yarn add package_name, it will update your package.json for you.

This is made by Facebook and is 10x+ faster than npm install.

Also it is included with Laravel installs if you use the laravel new command, so I figured it was good to add.

rappasoft commented 8 years ago

@blomdahldaniel I don't disagree with your frontend changes.

I think we should get all the backend code refactored and where we like it before we make GUI changes, as to not hold back the launch date of V4.

blomdahldaniel commented 8 years ago

Yes, it is a quite crazy PR.. :p But like it states, it is not supposed to be merged in as is but quickly show my idea of the frontend changes.

How should we go about it? Should I wait for some more backend changes or should I create a good solution (frontend and backend) and look for making a PR that should be mergable?

minedun6 commented 8 years ago

Can we get also to the fact that each time we composer require we need to manually config:cache ?

rappasoft commented 8 years ago

What do you mean?

rappasoft commented 8 years ago

Merged a few PR's into the upcoming release, what issues do we think are important enough to fix before this is released?

Also I need someone to run the current version and literally go through everything. Try all the functionality, look at the code, let me know if you find anything wrong.

num3thod commented 8 years ago

@rappasoft I'll try to give it a go later tonight.

blomdahldaniel commented 8 years ago

@rappasoft I'll have a go at it on Wednesday.

blomdahldaniel commented 8 years ago

I have now gone through the applications. I've made some small commits: 93707a8 Fix delete btn event binding for /logs 122bfaf add locale all "rights-reserved" (this might be dirty) b0e5e4e when resetting password, prefill email from token

And a PR that fixes a bug with LogViewer. I made it a PR since it is not probably the best solution but I could not find the error. So you may dig deeper and then see if you can solve it in another way.

506

I found a couple of big things that has to be adressed:

1. Missing translation for notification email:

I suggest adding most of these into locale --> strings.php

resources/views/vendor/notifications/email.blade.php

Following strings need to be added to locale:

app/Notifications/Frontend/Auth/UserNeedsConfirmation.php

public function toMail($notifiable){
    return (new MailMessage)
    ->subject(app_name() . ': ' . trans('exceptions.frontend.auth.confirmation.confirm'))
-   ->line(trans('strings.frontend.email.confirm_account'))
-   ->action('Confirm Account', route('frontend.auth.account.confirm', $this->confirmation_code))
-   ->line('Thank you for using our application!');
}

/vendor/laravel/framework/src/Illuminate/Auth/Notifications/ResetPassword.php

This one will have to be given locale as well:

public function toMail($notifiable)
{
    return (new MailMessage)
-        ->line('You are receiving this email because we received a password reset request for your account.')
-        ->action('Reset Password', url('password/reset', $this->token))
-       ->line('If you did not request a password reset, no further action is required.');
}

Also fix consistency compared to: ->subject(app_name() . ': ' . trans('exceptions.frontend.auth.confirmation.confirm')) So add the app_name() to subject.

resources/views/vendor/notifications/email-plain.blade.php

Needs same locale changes

2. Requested Improvement

Final thoughts

Some work has to be done regarding locale. Don't know if you want to do it yourself to feel comfortable about the variable names and stuff...?

I have tried to look for more things but cannot find anything more as of now. Everything looks great!

rappasoft commented 8 years ago

All your PR's look good.

If you have any time to do the stuff in your #1, it would be appreciated.

I'll work on #2.

As for locale, I've never made a real multilingual app so I don't know best practices, I think it works pretty well now, albeit messy, but functional.

Edit: Disregard the automatic number linking by GitHub, those are not correct in this instance

blomdahldaniel commented 8 years ago

Perf, I will deal with no 1 later this week

nasirkhan commented 8 years ago

I have some suggestions,

blomdahldaniel commented 8 years ago

1

No, if the user tries to log in without having confirmed his account, the user will get a flash message that says that the user has to confirm his account OR resend the confirmation email. How would you like to implement this in a better way?

2

Hm… Naah? The whole thing about Gravatar is that you shouln't have to upload or handle the profile picture at all. So all gravatar users should already have accepted this concept and receive that behaviour emediatly...? All websites I have signed up for that uses Gravatar has done it this way. Or do you have an example of a plattform that asks you whether you want to use the gravatar pic or use your own?

nasirkhan commented 8 years ago

1. I would like to have a reset link on the success alert message. Or it should be mentioned that "If you have not received the email then please try to login and you will get options to get another link." Even we may not redirect the user to a new page where this guideline will be mentioned.

I was not aware that this feature existed. :smile: As the success message said that confirm before login.

2. I like Gravatar but not all the users of the application might not have a Gravatar account. So there will be no way to update their profile photo. Yes, there is an example platform, https://forum.gitlab.com. register on this forum and try to change the profile photo from the settings. You will see 3 options, system generated, Gravatar, and upload file. I think we can implement this feature here.