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

Implements GNIP for curated-thumbs app #4731

Closed
wants to merge 2 commits into from

Conversation

capooti
Copy link
Member

@capooti capooti commented Aug 13, 2019

This PR implements the GNIP for curated-thumbs #4726

Here is an animated gif with the new feature:
http://g.recordit.co/JaKutevbrT.gif

It has an additional requirement: django-imagekit

Thumbnails are implemented for layers and maps. It would be very easy to implement them for documents as well but I see that there is already this feature (let me know if you want to replace it with this for homogeneity).

I decided to implement the application as an internal application for GeoNode as I believe this is useful for any GeoNode instance. If you believe it should rather go to a contrib application, I am ready to migrate it to geonode-contribs.

@codecov
Copy link

codecov bot commented Aug 13, 2019

Codecov Report

Merging #4731 into master will increase coverage by 0.06%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #4731      +/-   ##
==========================================
+ Coverage   59.49%   59.56%   +0.06%     
==========================================
  Files         230      236       +6     
  Lines       12261    12301      +40     
  Branches     1774     1776       +2     
==========================================
+ Hits         7295     7327      +32     
- Misses       4376     4383       +7     
- Partials      590      591       +1

Copy link
Member

@afabiani afabiani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @capooti nice feature, thanks, but do we really need to add another django app for this? Wouldn't be bettere to have this part of "base" and enable it for all the available resources?

@afabiani
Copy link
Member

afabiani commented Aug 14, 2019

... moreover, I don't see neither tests nor documentation. This is a brand new functionality which goes to core. I guess we need something more clean.

Copy link
Member

@afabiani afabiani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Some files are missing osgeo headers.
  • There's no documentation. At least the user guide should be updated.
  • No test present, we need at least one showing that actually, the thumbnails changed.
  • I would align Documents also. Why having such functionality only for Maps and Layers?
  • Any reason to have a specific app for that functionality? It this will be enabled by default, I don't see the need to have it separated

@@ -0,0 +1 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing headers

@@ -0,0 +1 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing tests

@capooti
Copy link
Member Author

capooti commented Aug 14, 2019

Thanks Alessio
having this in Base definitely makes sense. I will also include tests and documentation.
I am closing this PR and will send a PR
cheers

@capooti capooti closed this Aug 14, 2019
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.

2 participants