grant/cancel-contribute revamp #2946
Conversation
thelostone-mc
requested review from
owocki,
mbeacom,
willsputra and
captnseagraves
thelostone-mc
added
frontend
| @@ -35,87 +36,100 @@ | |||
| </div> | |||
| </div> | |||
| </div> | |||
| <img src="{% static "v2/images/bitmap.png" %}" width="100%" /> | |||
|
|
|||
There was a problem hiding this comment.
is it worth DRYing this up a bit?
DRY = dont repeat yourself
There was a problem hiding this comment.
Ah I should do that 🙌
There was a problem hiding this comment.
@owocki is there a specific place you see the need for DRY or just the file in general?
There was a problem hiding this comment.
@captnseagraves I believe this was what @owocki was talking about
904a865
There was a problem hiding this comment.
@owocki is there a specific place you see the need for DRY or just the file in general?
the copy/pasted 'warning: were in alpha' warning
|
|
||
| <div class="col-12 grant-banner py-5 row mx-0"> | ||
| <div class="col-12 offset-md-2 col-md-3 py-4 banner-img"> | ||
| <img src="{% if grant.logo and grant.logo.url %}{{ grant.logo.url }}{% else %}{% with grant_logo='v2/images/grants/logos/' id=grant.id|modulo:3|add:1 %} {% static grant_logo|addstr:id|add:'.png' %} {% endwith %} {% endif %}"> |
There was a problem hiding this comment.
id=grant.id|modulo:3|add:1
static grant_logo|addstr:id|add:'.png'
this logic seems unnecessarily complicated to put into the template. what is it doing? why can't we create a parameter off of the grant obj?
There was a problem hiding this comment.
So since we weren't saving an image if the grant owner doesn't add it! We've map it to one of the 4 static assets (default images) we have.
to ensure the same default always loads -> I just go grant_id % 3 + 1 to map it to the file
I know it's meh 😓 could use some help with that.
but ideally that code shows this
There was a problem hiding this comment.
i really like the design. but still think the logic for what image is shown could be encapsulated into a method off of the grants object.
|
left a few comments. screenshot looks good 👌 |
There was a problem hiding this comment.
Looking good. Just want to hear back from @owocki on his comment.
| @@ -35,87 +36,100 @@ | |||
| </div> | |||
| </div> | |||
| </div> | |||
| <img src="{% static "v2/images/bitmap.png" %}" width="100%" /> | |||
|
|
|||
There was a problem hiding this comment.
@owocki is there a specific place you see the need for DRY or just the file in general?
| @@ -14,12 +14,13 @@ | |||
| You should have received a copy of the GNU Affero General Public License | |||
| along with this program. If not, see <http://www.gnu.org/licenses/>. | |||
| {% endcomment %} | |||
| {% load static humanize i18n %} | |||
| {% load static humanize i18n grants_extra %} | |||
There was a problem hiding this comment.
What is grants_extra doing? I haven't seen this yet.
There was a problem hiding this comment.
@captnseagraves #2946 (comment)
I would appreciate some help making this nicer 😅
There was a problem hiding this comment.
grants_extra has modulo helper function #2946 (comment)
There was a problem hiding this comment.
@thelostone-mc could we put this logic in the grants/views.py grant_new function at ln160? Pseudocode: if logo == None perform logic at ln65 of grants/cancel. This address this issue on object creation and puts the logic in a better place than the template.
Codecov Report
@@ Coverage Diff @@
## grants #2946 +/- ##
=======================================
Coverage 17.05% 17.05%
=======================================
Files 175 175
Lines 13864 13864
Branches 1813 1813
=======================================
Hits 2365 2365
Misses 11491 11491
Partials 8 8Continue to review full report at Codecov.
|
thelostone-mc
force-pushed
the
grants
branch
from
904a865 to
dfd97b2
Compare
|
I basically needed a modulo 3 of the grant id to decide which static asset to map to it to and ensure that it stays the same 😅 |
refs: gitcoinco#2949
thelostone-mc
force-pushed
the
grants
branch
from
6311621 to
7ec3641
Compare
Description