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

Separate Dragonfly from Images and Resources. #3289

Merged
merged 4 commits into from Oct 18, 2017

Conversation

Projects
None yet
4 participants
@anitagraham
Contributor

anitagraham commented Apr 16, 2017

Modify Images and Resources to use common Dragonfly 'engine'
Remove Dragonfly initialization from core to Dragonfly
Remove Dragonfly testing from core to Dragonfly
Add tests to Images and Resources to test Dragonfly interface
Change Generators for Core, Dragonfly, Images and Resources to
reflect configuration changes for each one.

Dragonfly can be used for other engines using the same config options as Images/Resources

Dragonfly can be put into a separate gem, but it will be easier to review this change
with everything in the same repository.

@anitagraham

This comment has been minimized.

Show comment
Hide comment
@anitagraham

anitagraham Apr 17, 2017

Contributor

Hmmm... lots of xhr paging failures, but a few Resources/veryify_urls issues which I will follow up.

Contributor

anitagraham commented Apr 17, 2017

Hmmm... lots of xhr paging failures, but a few Resources/veryify_urls issues which I will follow up.

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Jul 15, 2017

Member

@anitagraham looks like the test failures were unrelated, as I fixed them the other day. Are you still happy with this change?

Member

parndt commented Jul 15, 2017

@anitagraham looks like the test failures were unrelated, as I fixed them the other day. Are you still happy with this change?

@bricesanchez

Thanks @anitagraham for your awesome work!

I like the fact to extract dragonfly from refinery but i think we should go further :

  • Should we keep the name refinerycms-dragonfly ? Should not we use a more generic name like refinerycms-uploader? because if tomorrow i want to use shrine, what happens?
  • We should create a new repository with this extract like we did with refinerycms-authentication-devise it will keep this repository cleaner :)

Any thoughts @anitagraham and @parndt ?

def dragonfly_custom_backend_class
config.dragonfly_custom_backend_class.constantize if dragonfly_custom_backend?
raise "Refinery::Dragonfly now handles all dragonfly configuration. Consult 'config/initializers/refinery/dragonfly.rb'."

This comment has been minimized.

@bricesanchez

bricesanchez Sep 30, 2017

Member

You should deprecate this method instead of raising an error.

@bricesanchez

bricesanchez Sep 30, 2017

Member

You should deprecate this method instead of raising an error.

This comment has been minimized.

@anitagraham

anitagraham Oct 2, 2017

Contributor

Right now this is refinerycms-dragonfly. Next release we can split it into a separate gem (refinerycms-dragonfly) and an uploader and a generic image handler.

I absolutely agree that we should move further away from dragonfly - this is just one step. And once it is out there working, creating it as an external gem is easy.

To move away from Dragonfly altogether we need to think about how we handle image resizing and conversion - at the moment it is all done using ImageMagick geometry. We probably need to abstract this enough to handle other graphics solutions (Cloudinary for instance).

Comments about repositories etc all fine.

@anitagraham

anitagraham Oct 2, 2017

Contributor

Right now this is refinerycms-dragonfly. Next release we can split it into a separate gem (refinerycms-dragonfly) and an uploader and a generic image handler.

I absolutely agree that we should move further away from dragonfly - this is just one step. And once it is out there working, creating it as an external gem is easy.

To move away from Dragonfly altogether we need to think about how we handle image resizing and conversion - at the moment it is all done using ImageMagick geometry. We probably need to abstract this enough to handle other graphics solutions (Cloudinary for instance).

Comments about repositories etc all fine.

This comment has been minimized.

@bricesanchez

bricesanchez Oct 2, 2017

Member

@anitagraham I agree with you too, could you update your PR with the minor changes i requested?

@bricesanchez

bricesanchez Oct 2, 2017

Member

@anitagraham I agree with you too, could you update your PR with the minor changes i requested?

Show outdated Hide outdated images/refinerycms-images.gemspec Outdated

@bricesanchez bricesanchez added this to the 4.1 milestone Sep 30, 2017

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Oct 1, 2017

Member

I like the fact to extract dragonfly from refinery but i think we should go further :

Should we keep the name refinerycms-dragonfly ? Should not we use a more generic name like refinerycms-uploader? because if tomorrow i want to use shrine, what happens?
We should create a new repository with this extract like we did with refinerycms-authentication-devise it will keep this repository cleaner :)

I think it'd be good to get this merged and then extract it, in the spirit of getting it happening and not getting it delayed again, what do you think?

Member

parndt commented Oct 1, 2017

I like the fact to extract dragonfly from refinery but i think we should go further :

Should we keep the name refinerycms-dragonfly ? Should not we use a more generic name like refinerycms-uploader? because if tomorrow i want to use shrine, what happens?
We should create a new repository with this extract like we did with refinerycms-authentication-devise it will keep this repository cleaner :)

I think it'd be good to get this merged and then extract it, in the spirit of getting it happening and not getting it delayed again, what do you think?

@anitagraham

This comment has been minimized.

Show comment
Hide comment
@anitagraham

anitagraham Oct 11, 2017

Contributor

Getting onto it!

Contributor

anitagraham commented Oct 11, 2017

Getting onto it!

@anitagraham

This comment has been minimized.

Show comment
Hide comment
@anitagraham

anitagraham Oct 12, 2017

Contributor

OK. think I've made the requested change. It passed testing locally.

Contributor

anitagraham commented Oct 12, 2017

OK. think I've made the requested change. It passed testing locally.

@parndt

Finally had time to go through this line by line - thanks for doing this!

@anitagraham

This comment has been minimized.

Show comment
Hide comment
@anitagraham

anitagraham Oct 12, 2017

Contributor

Requested changes made (except one which is the least important, that I missed.)
Thanks for reviewing it.

Next: take it out to a gem.
Next+1: - if not immediately - is to think about generalising the image geometry/config etc so other image management can be used.

Contributor

anitagraham commented Oct 12, 2017

Requested changes made (except one which is the least important, that I missed.)
Thanks for reviewing it.

Next: take it out to a gem.
Next+1: - if not immediately - is to think about generalising the image geometry/config etc so other image management can be used.

@bricesanchez

I point you some minor changes. One more time that's a really good job @anitagraham !

Show outdated Hide outdated Gemfile Outdated
def dragonfly_custom_backend_class
config.dragonfly_custom_backend_class.constantize if dragonfly_custom_backend?
raise "Refinery::Dragonfly now handles all dragonfly configuration. Consult 'config/initializers/refinery/dragonfly.rb'."

This comment has been minimized.

@bricesanchez

bricesanchez Oct 13, 2017

Member

I keep my comment about "You should deprecate this method instead of raising an error." :)

@bricesanchez

bricesanchez Oct 13, 2017

Member

I keep my comment about "You should deprecate this method instead of raising an error." :)

This comment has been minimized.

@anitagraham

anitagraham Oct 13, 2017

Contributor

Sorry - didn't read it properly last time. Done now.

@anitagraham

anitagraham Oct 13, 2017

Contributor

Sorry - didn't read it properly last time. Done now.

Show outdated Hide outdated dragonfly/refinerycms-dragonfly.gemspec Outdated
Separate Dragonfly from Images and Resources.
Modify Images and Resources to use common Dragonfly 'engine'
Remove Dragonfly initialization from core to Dragonfly
Remove Dragonfly testing from core to Dragonfly
Add tests to Images and Resources to test Dragonfly interface
Change Generators for Core, Dragonfly, Images and Resources to
reflect configuration changes for each one.

Dragonfly can be used for other engines using the same config options as Images/Resources

Dragonfly could be moved out of Refinery altogether, but it will be easier to review this change
with everything in the same repository.

Add generator templates. Specs again
@anitagraham

This comment has been minimized.

Show comment
Hide comment
@anitagraham

anitagraham Oct 13, 2017

Contributor

Done again - and I do appreciate the detailed reviews and the thanks.

Contributor

anitagraham commented Oct 13, 2017

Done again - and I do appreciate the detailed reviews and the thanks.

@anitagraham

This comment has been minimized.

Show comment
Hide comment
@anitagraham

anitagraham Oct 16, 2017

Contributor

? Is there a way for me to just say yup and have each of those changes/reviews accepted, or do I take each one and apply it to my local copy, then pass it back here?

Contributor

anitagraham commented Oct 16, 2017

? Is there a way for me to just say yup and have each of those changes/reviews accepted, or do I take each one and apply it to my local copy, then pass it back here?

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Oct 16, 2017

Member

@anitagraham the easiest way is just to change it locally and add a new commit with that change which you then push to the branch.

@bricesanchez has also opened a PR for you which you can merge if you like: anitagraham#4

Member

parndt commented Oct 16, 2017

@anitagraham the easiest way is just to change it locally and add a new commit with that change which you then push to the branch.

@bricesanchez has also opened a PR for you which you can merge if you like: anitagraham#4

@simi

simi approved these changes Oct 16, 2017

@parndt parndt merged commit 39a253e into refinery:master Oct 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Oct 18, 2017

Member

It's merged 🎉 thanks @anitagraham

Member

parndt commented Oct 18, 2017

It's merged 🎉 thanks @anitagraham

@bricesanchez

This comment has been minimized.

Show comment
Hide comment
@bricesanchez

bricesanchez Oct 18, 2017

Member

🎉 🍻 Thanks @anitagraham !

Member

bricesanchez commented Oct 18, 2017

🎉 🍻 Thanks @anitagraham !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment