Improvements on console UI for install command #4559
Conversation
[ci skip] [skip ci]
…Laravel-Backpack/CRUD into install-console-ui-improvements
|
Key points of this PR.
To add new addons to the the list, it's as easy as add the require command class to this list;
It must follow a pattern, but it's also a matter of copy past from the others. |
|
Another important note, since this inclues installation of paid addons, every paid addon must make sure about the same stuff;
Since that was done in DevTools require command, I used the same code, which made everything MUCH simpler. Each Require addon command class is now less than 100 lines, and is readable 😌👌 #3843 may be closed by merging this one. |
There was a problem hiding this comment.
I FREAKING LOVE THIS! ❤️❤️❤️ Well done @promatik !
Beautiful & functional 👏👏👏
I want to merge this ASAP, so let's clean it up please 🙏 I have one big problem with it (the error I'm getting), other than that, I just changes the texts here and there. I want to test again after that error is fixed, then merge ASAP - this is a HUUUGE DX improvement.
Co-authored-by: Cristian Tăbăcitu <[email protected]>
…Laravel-Backpack/CRUD into install-console-ui-improvements
There was a problem hiding this comment.
I LOVE IT! I love it, I love it, I love it! ❤️❤️❤️ Thanks @promatik !
I'm noticing a few design/visual changes we should be making here, to give the whole install process a coherent look & feel, mainly in terms of visual hierarchy.
But other than that... this is working WONDERFUL for me 👏👏👏
Much MUCH better than before.
…Laravel-Backpack/CRUD into install-console-ui-improvements # Conflicts: # src/app/Console/Commands/Install.php
[ci skip] [skip ci]
Removed questions after being answered
[ci skip] [skip ci]
|
Hey guys. First of all, nice work @promatik, I think this "beauty terminal" has some real value! 1) I think it would be usefull to have default values for the user questions (name/email/password). 2) I think it would help in the "resume of created user" to include the password too so it could be copy-pasted: 3) Would it be cumbersome to add some validation ? Nothing fancy, just like:
Avoiding things like: 4) Wouldn't be better to add some more "focus" on the numbers for the addons ? Maybe I will review the code in a separate thread. Cheers |
| $this->note('(Find your access token details on https://backpackforlaravel.com/user/tokens)'); | ||
| $username = $this->ask('Access token username'); | ||
| $password = $this->ask('Access token password'); | ||
|
|
There was a problem hiding this comment.
We know we have some "pitfalls" when user adds wrong authentication, and then it's cached and then you need to dabble a little bit in your brain how to fix it (the issues we found in the other PR this one replaced).
I think it would atleast help if we do minimal validation here like, not empty and atleast min:xx characters (the number configured in backpack application to generate tokens).
It will not solve all the problems but atleast will prevent "simple" mistakes:
- pressing enter twice;
- not copying correctly from backpack website;
- not pasting correctly on the console;
There was a problem hiding this comment.
Nice idea, will do it 👌
I'll add simple validations to all the inputs.
There was a problem hiding this comment.
I agree. But let's make a separate issue please. This doesn't relate to the current PR, it's an unrelated improvement.
|
In regards to Pedro's comment above:
I'd rather not add defaults at all. I don't want people creating the stock admin, and knowing you can "probably" try that email or email/password combo in all Backpack admin panels. Let them choose their own admin & password, no defaults.
Maaaybe... I don't know... I'll let @promatik decide about this one. Feels odd to echo the password, when the field itself will not echo the password.
Yeah we should probably add validation. Bonus points if we re-use the validation from Registration or something. Then again this would make the whole process a lot more difficult, maybe you actually WANT to add an admin on localhost that is "admin" and "password" just so it's easy. It's localhost after all. So maybe we just add
Yeah I also wish the numbers would be a little more evident. |
|
@promatik let's wrap this up, I want to merge and tag it ASAP please 🙏 Please add separate issues for any other suggestions here, and we'll do them one-by-one. Nothing I see here is worth postponing the merge, in my opinion - not even my suggestions. To rephrase the importance: Every day we delay merging this, we offer a sub-par installation experience to hundreds of devs. When we have a much much better one ready! Is this ready for me to click the merge button?
|
|
Apart from what I already wrote, this is working fine, you can hit that button ✅ |
|
@promatik - awesome, yes, please do send a separate PR for that. I've just MERGED this. I'm so so glad about having this, some might call this small but it is a pretty BIG improvement. And a BIG step in the right direction. Let's polish this onboarding process some more 👏👏👏 Thanks guys! |
|
Writing about this in our Progress Report. So so happy with it ❤️ Just look at this beauty: |
WHY
BEFORE - What was wrong? What was happening before this PR?
This PR aims part of the idea reported on Laravel-Backpack/community-forum#163.
AFTER - What is happening after this PR?
Backpack installation is now much more complete, and in sync with the latest console UI changes on Laravel 9.21 — laravel/framework#43065.
Click below to expand more previews;
More previews
2022-07-30.22-12-11.mp4
HOW
How did you achieve that, in technical terms?
Implementing manually the features we need from termwind (since it supports PHP8 only), it was not that hard since we need only a couple of them.
Is it a breaking change?
No 👌
How can we test the before & after?
Following the video above, and changing the branch of crud after it was installed.