-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
Media Management #180
Media Management #180
Conversation
…bansal/media-manager package
I've got no idea how to fix that Version Eye check, as far as I can tell I've set it in my composer.json file and have LICENSE file in my project root :( |
No worries @talvbansal, VersionEye is seeing no license in @erikgall eloquent/phpunit package either. It's not a breaking thing. All the dependencies should still be up to date, I'll still be able to merge without that passing. |
Can't wait to check this out! Just gotta make sure I write some sort of upgrade path now for those who already are using Canvas, but it looks like you detailed it out pretty well. |
Awesome! I cant wait to hear what you think! |
@austintoddj @talvbansal I added a license to the package's Update: It looks like they finally did. It just took a little bit. |
@talvbansal Looking at the PR right now. If you go to the UPDATE: Nevermind on that last comment. Local problem with Valet. Still going through the PR now. Some UI tweaks here and there that I'll want to apply, but it's looking nice. If I put together a 'fix-list', would that be a good way to make a few last changes before we pull the trigger? |
Sure thing, can't wait to see the fix list 😀 |
Ok, first off, I want to thank you @talvbansal, this is an AWESOME feature! Really enjoyed going through this, and I think it makes Canvas so much better than it was before. The changes I listed below aren't anything major, they mainly detail how this can fit in the current UI/UX a little better. Let me know your thoughts and how feasible they are, or if they are unclear in any way. Again, thanks for making this PR, can't wait to merge it in. Post Page
|
… for active media-manager rows match -primary in new backend variables.scss file
…t inside the window as they would on the post page when rendered
Thanks for checking it out and for all of your feedback! I've made all of your recommended changes with the exception of the following:
As the media manager is supposed to be used in other projects (I am in a few work projects already) I need to keep it portable and so need to bundle the icons with it, I'd be happy to change the icons provided that they were SVG's and not font icons. Listen to this for an overview on why SVG's are better. I think its important to have some visual feedback for potentially long running operations like refreshing the file list or uploading so I've decided to keep the loading icon, however it was too big before so I've reduced it in size and styled it to match Canvas's primary colour as well as adding the text "Loading..." after it. Take a look and once we're done with this I'll make the change for scroll to top mentioned in #179 - just didn't want to put two features into this PR |
Awesome, thanks @talvbansal! I'll look at this today and we'll keep this moving. |
@talvbansal Just looked it over, and am super happy with the way this turned out. Last fix: Looks like the modal that the media manager uses clashes just a bit with the other modals on the site, hopefully this wouldn't make it unusable for your other projects if you made the Media Manager modals look a bit more like the rest on Canvas. Here is a screen shot of your modal: And here is one of Canvas' modals: So it looks like the padding/size might need to be addressed to give it some breathing room, the buttons, the element, and icons on the delete button. Maybe more, but that's what I could see right away. Would you be able to make it a little more closely aligned with the others? If so, then that should be the last fix before I merge this in 👍 |
Yep totally fine with doing that, I'll take a look at it now |
Take a look now, they should match the existing modal windows :) |
Replaced file management with talvbansal/media-manager which should resolve issue #19
However there is one change that may impact some users.
The media manager will look in
/storage/app/public
folder as its root folder, and a link should be created from here to topublic/storage
using thephp artisan storage:link
command (see here for more )[https://laravel.com/docs/5.3/filesystem#the-public-disk].The consequence of this is that in Canvas none of the existing images will work.
To fix this the following steps should be taken:
public/uploads
folder to thestorage/app/public
folder.php artisan migrate
(I've created a migration to prefix $post->page_image with /storage/ where necessary and a rollback to undo the change)Hope you like it!