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

#4321 Update Links for Marketing #4341

Merged
merged 5 commits into from
May 12, 2019
Merged

#4321 Update Links for Marketing #4341

merged 5 commits into from
May 12, 2019

Conversation

kuhnchris
Copy link
Contributor

Description

See #4321

Refers/Fixes

Fixes: #4321

Testing

@codecov
Copy link

codecov bot commented May 1, 2019

Codecov Report

Merging #4341 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4341      +/-   ##
==========================================
- Coverage    30.2%   30.16%   -0.04%     
==========================================
  Files         209      209              
  Lines       16776    16776              
  Branches     2245     2245              
==========================================
- Hits         5067     5061       -6     
- Misses      11519    11525       +6     
  Partials      190      190
Impacted Files Coverage Δ
app/dashboard/embed.py 28.16% <0%> (-3.45%) ⬇️

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 79b1cdb...c4f08a2. Read the comment docs.

@codecov
Copy link

codecov bot commented May 1, 2019

Codecov Report

Merging #4341 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4341   +/-   ##
=======================================
  Coverage   30.19%   30.19%           
=======================================
  Files         209      209           
  Lines       16790    16790           
  Branches     2249     2249           
=======================================
  Hits         5070     5070           
  Misses      11527    11527           
  Partials      193      193
Impacted Files Coverage Δ
app/retail/views.py 29.6% <ø> (ø) ⬆️

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 3f8f5c1...bf27d6b. Read the comment docs.

SaptakS
SaptakS previously requested changes May 3, 2019
app/retail/templates/bounties/contributor/story.html Outdated Show resolved Hide resolved
@kuhnchris
Copy link
Contributor Author

@SaptakS Thanks for checking, please check again, better now?

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

left comments

<a href="/contributor/design">Design</a>,
<a href="/contributor/html">HTML</a>,
<a href="/contributor/ruby">Ruby</a>,
<a href="/contributor/css">CSS</a>,
Copy link
Member

Choose a reason for hiding this comment

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

Could we pass this from the backend as an array and simply loop through it here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory yes, I also thought of that, but we'd need to create a new either table or json in python.
What route do you prefer? Table for editing live, or json/hardcoded json to have a deploy cycle?

Copy link
Member

Choose a reason for hiding this comment

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

Harcoded Json for now would work 🙌

Copy link
Contributor

Choose a reason for hiding this comment

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

@kuhnchris You can just send it as part of the template params as an array of strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, just wanting to make sure that's reaaaaaally what you want. But sure, i'll add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole bounties/contributor stuff is a whole lot of 💩 .
we cannot use 'url' with that, since it's not a registered route but somehow dynamically generated.

django.urls.exceptions.NoReverseMatch: Reverse for 'bounties/contributor' not found. 'bounties/contributor' is not a valid view function or pattern name.
     {% for subject in contributor_list %}
          <a href="{% url 'bounties/contributor' %}"{=subject.link=}>{=subject.text=}</a>,  <- neither
        {% endfor %} 
        <a href="{% url 'contributor/python' %}">Python</a>,  <- nor this
        <a href="{% url 'contributor' %}/python">Python</a>,  <- nor this

Please advice.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not whole lot of 💩 , you are just writing it wrong. The code is supposed to be:

{% for subject in contributor_list %}
     <a href="{% url 'contributor_bounties' tech_stack=subject.link %}">{{ subject.text }}</a>,  <- neither
 {% endfor %} 

This is assuming that subject.link is /python and subject.text is Python

I am not sure where you got the above syntax from. I would really advice you to go through this documentation to learn django templating: https://docs.djangoproject.com/en/2.2/topics/templates/

Copy link
Contributor Author

@kuhnchris kuhnchris May 3, 2019

Choose a reason for hiding this comment

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

@SaptakS Thanks for the heads up, still thinking that contributor + tech_stack is 💩 (especially cause I need to comment views.py L567-568 just to make it work on a local dev environment, else those pages 404) , but I couldn't care less.
grafik

Also, thanks for relinking me to the template documentation, I tried to get the url '' part working so I tried working with various types of {= {% ... to ensure it wasn't me fucking this up.

Committed, please check, worked over here.
grafik

Just letting you know for some reason you dropped "Go" but it's on the infographics on the left, so I might reconsider dropping it. @PixelantDesign

Copy link
Member

Choose a reason for hiding this comment

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

^ @PixelantDesign let us know if that needs to be updated

@thelostone-mc thelostone-mc requested a review from SaptakS May 12, 2019 09:32
@thelostone-mc thelostone-mc merged commit d47f8e6 into gitcoinco:master May 12, 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.

Marketing - Update Links on Contributor Landing Pages
3 participants