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

[WIP] Added tabs to grant details #3505

Merged
merged 9 commits into from Jan 15, 2019

Conversation

@usmanmuhd
Copy link
Contributor

commented Jan 11, 2019

Fixes: #3286
TODO: Subscriptions
image


image

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:grant-detail-tabs branch from e3c80c5 to f4842aa Jan 11, 2019

@codecov

This comment has been minimized.

Copy link

commented Jan 11, 2019

Codecov Report

Merging #3505 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3505      +/-   ##
=========================================
- Coverage   29.61%   29.6%   -0.01%     
=========================================
  Files         185     185              
  Lines       14684   14686       +2     
  Branches     1942    1942              
=========================================
  Hits         4348    4348              
- Misses      10197   10199       +2     
  Partials      139     139
Impacted Files Coverage Δ
app/grants/views.py 14.11% <0%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 657b97a...8b84d59. Read the comment docs.

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:grant-detail-tabs branch 2 times, most recently from 0b0baff to 096ce94 Jan 11, 2019

@owocki

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

@usmanmuhd very excited for this! can you code in the transaction history today too?

@thelostone-mc thelostone-mc self-requested a review Jan 11, 2019

@usmanmuhd

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

@owocki on it..

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:grant-detail-tabs branch from 096ce94 to d647dec Jan 11, 2019

@usmanmuhd

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

Could I get the design for the way subscriptions should be displayed?

@owocki

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

@usmanmuhd there is a design on the spec ticket #3286

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:grant-detail-tabs branch 2 times, most recently from 3978822 to 8730711 Jan 12, 2019

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:grant-detail-tabs branch 2 times, most recently from 30d6769 to e59d624 Jan 13, 2019

@usmanmuhd usmanmuhd force-pushed the usmanmuhd:grant-detail-tabs branch from e59d624 to b22b6f4 Jan 13, 2019

thelostone-mc added some commits Jan 14, 2019

@thelostone-mc

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

When there is no activity

screen shot 2019-01-14 at 11 04 46 pm

When there is activity

screen shot 2019-01-14 at 11 03 19 pm

@usmanmuhd I split up the code into smaller templates

details/
    index.html
    summary.html
activity.html

@captnseagraves could you pull this in and tell me if looks okay at your end too!
@usmanmuhd mind doing a sanity test to make sure I haven't broken your code?

@owocki

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

agree with @thelostone-mc !

@owocki

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

just tested this... its looking good :)

@owocki

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

@owocki
Copy link
Member

left a comment

coming along good.. i just left one comment

app/assets/v2/js/grants/index.js Outdated Show resolved Hide resolved
<i class="g-icon g-icon__dot-circle mr-2"></i>{% trans "Active Subscriptions" %}
</p>
{% for subscription in subscriptions %}
<div class="py-3 mx-sm-0 row transaction-history">

This comment has been minimized.

Copy link
@owocki

owocki Jan 14, 2019

Member

any way we can DRY up these transaction-history divs? seems like there is a lot of pasted code in them, and they coudl be one shared template

This comment has been minimized.

Copy link
@owocki

owocki Jan 14, 2019

Member

cancelled-subscriptions, active-subscriptions, and contributions

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc Jan 14, 2019

Member

I did wanna do that but when sat with it and realized it becomes a tad bit messy cause

{% for transaction in contributions %}
  {{ transaction.id }} ...
  {{transaction.subscription.X}} ...

& we have

{% for subscription in cancelled_subscriptions %} // same for active-subscriptions
  {{subscription.id}} ...
  {{subscription.X}} ...

So this would mean the template would have a lot of ifs and buts which made the code a lot less readable... so after 30 min -> git stash && git stash drop 😓

This comment has been minimized.

Copy link
@owocki

owocki Jan 14, 2019

Member

have you considered making a subtemplate that has a bunch of variables like so

<div>
  <a href="{{url}}">{{title}}</a>
</div>

and then calling it like so

{% include 'subtemplate.html' with url='foo' title='bar'

?

decoupling the format of the template to variables like above would allow you to DRY and not have a bunch of complex if statements in the template..

@thelostone-mc

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

@owocki sorry about that ! got the mobile fixes in

untitled

@owocki

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

excited to get this live! think we'll be close by tomorrow?!

@thelostone-mc

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

@owocki / @usmanmuhd all we need is another round of sanity testing and we should we be good to go 🙌

@thelostone-mc

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

@usmanmuhd

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

Tested it, looks really good.
Thanks for the design :)

@owocki owocki merged commit 212bd58 into gitcoinco:master Jan 15, 2019

1 of 3 checks passed

codecov/patch 0% of diff hit (target 29.61%)
Details
codecov/project 29.6% (-0.01%) compared to 657b97a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@usmanmuhd usmanmuhd deleted the usmanmuhd:grant-detail-tabs branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.