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

Extending the FileBrowser backend to add support for ImageField #101

Merged
merged 1 commit into from Aug 15, 2016
Merged

Extending the FileBrowser backend to add support for ImageField #101

merged 1 commit into from Aug 15, 2016

Conversation

fgmacedo
Copy link
Contributor

The current file-browser backend only has support for the FileBrowserField. This PR enables the use of FileBrowser as backend with regular ImageFeld fields.

PS: The file-browser was published on pypi with the changes to add django-image-cropping support.

@anrie anrie merged commit 3c98874 into jonasundderwolf:master Aug 15, 2016
@anrie
Copy link
Member

anrie commented Aug 15, 2016

Hey @fgmacedo ,

I plan to release a new version that incorporates your FileBrowser backend in the next days.

I quickly tried to test the backend as I haven't used django-filebrowser before but I ran into some problems. Where am I supposed to crop the image? In the change view of an image?
If I try to edit an image (/admin/example/image/5/change/) I get the following error Reverse for 'grp_related_lookup' with arguments '()' and keyword arguments '{}' not found. 0 pattern(s) tried: []
Maybe I just missed something when setting up the app but it's probably a good idea to make the docs a bit more explicit.

On the other hand we currently repeat most of the documentation form django-filebrowser:

https://github.com/jonasundderwolf/django-image-cropping#django-filebrowser vs.
https://django-filebrowser.readthedocs.io/en/latest/quickstart.html#installation.

It's probably safe to assume that only people who are already familiar with django-filebrowser will use the backend and therefore I would skip the detailed explanation and just refer to the original docs. What do you think?

Finally I really would like to have some basic integration tests that show that both backends work as advertised and switching backends work. Especially as we are currently not using the FileBrowser backend ourselves it would be good to have some checks so we don't break stuff unintentionally while the app progresses.

@fgmacedo
Copy link
Contributor Author

fgmacedo commented Aug 15, 2016

Hi @anrie ! Thanks for your reply.

Good to know about the release. Please let me know if you need something.

You probably forgot to add grappelli urls into your urls.py:

url(r'^grappelli/', include('grappelli.urls')),

I totally agree with you about the setup instructions, and about the integration tests.

I'm able to submit another PR with filebrowser related tests. But while I was reading your reply, I realize that the major efforts were put to isolate backend specific logic under a facade.

And since the backend config is a python path for a class, and that all the code that is suposed to run only for django-filebrowser is under backends/fb.py, the backends/fb.pyis shipping with django-image-cropping only for convenience, not by need.

What do you think of instead a PR to add tests, a PR to wipe out django-filebrowser related stuff from django-image-cropping? And we put the custom backend inside another repository, with tests and a basic example project? I can keep this repository up and running, since I'm already using this integration. And you are free from having to maintain code that you are not planning to run.

We can open a section on django-image-cropping's readme to list third-party backends, so maybe in the future I'm not the only one to use django-image-cropping with django-filebrowser.

Please let me know.

Best regards,

@anrie
Copy link
Member

anrie commented Aug 15, 2016

Hey @fgmacedo,

I actually had the same idea. I think moving the backend to its own repository is a really good idea as it makes maintaining things a lot simpler. We don't ship code that we don't use and you can iterate much quicker if you plan to extend the backend.

Maybe we could use something like extra_require (http://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies) to allow users to additionally install the desired backends?

I guess we only need a convention/schema to declare compatible version.

Thanks a lot for your efforts!

@fgmacedo
Copy link
Contributor Author

Nice!

I think that extra_require fits perfectly.

Something like:

pip install django-image-cropping[filebrowser]

I'll work on this in the next days.

@fgmacedo fgmacedo deleted the filebrowser-with-filefield branch August 15, 2016 16:04
@sonarun
Copy link

sonarun commented Sep 1, 2016

Is it possible to implement this using custom actions?

Like anrie, after implementing the FileBrowser backend, I have no idea where to actually crop the image. In Filebrowser, you can create an "action" on an item and I think that would be the best place to pop a modal or something to actually crop the image.

anrie added a commit that referenced this pull request Sep 25, 2016
We rather keep the backends in separate packages.

See PR #101 for the relevant discussion.
@anrie
Copy link
Member

anrie commented Sep 25, 2016

@fgmacedo I now removed the filebrowser backend from the main package and adjusted the docs accordingly. Let me know when you have repo/package for the additional backends so we can refer to them in the README. Have a nice sunday!

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.

None yet

3 participants