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

Add democratic commons links #15652

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

wrightmartin
Copy link
Contributor

@wrightmartin wrightmartin commented Apr 3, 2018

What does this do?

Adds information to the EveryPolitician website about the Democratic Commons project it is now part of. It's barebones and I expect when assets exist we will come back and add a logo or graphic treatment.

Why was this needed?

Previously there were no links at all

Relevant Issue(s)

Implementation notes

None, just basic HTML and CSS changes

Screenshots

Homepage

screen shot 2018-04-03 at 15 51 37

Country page

screen shot 2018-04-03 at 15 51 30

Notes to Reviewer

Notes to Merger

@wrightmartin wrightmartin requested a review from tmtmtmtm April 3, 2018 14:54
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15652 April 3, 2018 14:54 Inactive
@wrightmartin wrightmartin force-pushed the add-democratic-commons-links branch from 691ac99 to 266ef58 Compare April 3, 2018 14:58
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15652 April 3, 2018 14:58 Inactive
Copy link
Contributor

@tmtmtmtm tmtmtmtm left a comment

Choose a reason for hiding this comment

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

Thanks for this @wrightmartin

There are a few problems here, though:

  • travis is failing:
    • Note that this repo has an enforced check that any non-refactoring PR adds a test that not only passes on the branch, but also fails on master. In this case that would probably be a matter of making sure that one of the country pages has a link to mysociety.org/democracy/democratic-commons (I'd be happy to add something like this myself if you're not comfortable doing that)
  • you've left a fixup commit in: you should squash that
  • there's a typo on "contribute"

More fundamentally, though, I'm not convinced that this change makes sense in isolation. It gives a quite confusing message about the project, particularly as it's placed right next to a "How to contribute" link that mentions nothing at all about Wikidata or the Commons, and indeed gives conflicting information.

I've opened everypolitician/everypolitician-docs#43 to track the larger rewrite: we don't necessarily need to block this on all of that, but I don't think we can put this live without making at least some additional changes.

</div>
<div class="column-one-third">
<h3>Contribute data</h3>
<p>Missing data for your country? Here's how to get that changed:</p>
<a href="http://docs.everypolitician.org/contribute.html" class="button button--small button--secondary">Read more…</a>
<a href="http://docs.everypolitician.org/contribute.html">How to contribue</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

contribute

@wrightmartin wrightmartin force-pushed the add-democratic-commons-links branch from 266ef58 to 7717114 Compare April 4, 2018 11:32
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15652 April 4, 2018 11:32 Inactive
@wrightmartin
Copy link
Contributor Author

Thanks for the feedback @tmtmtmtm - typo fixed, fixup gone.

I've not written a test before but I took a run at it - see what you think

@wrightmartin wrightmartin force-pushed the add-democratic-commons-links branch from 7717114 to 6d97a41 Compare April 4, 2018 11:33
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15652 April 4, 2018 11:33 Inactive
@wrightmartin
Copy link
Contributor Author

wrightmartin commented Apr 4, 2018

More fundamentally, though, I'm not convinced that this change makes sense in isolation. It gives a quite confusing message about the project, particularly as it's placed right next to a "How to contribute" link that mentions nothing at all about Wikidata or the Commons, and indeed gives conflicting information.

Make sense to me - but in this case you're talking to the monkey and not the organ grinder - @rich-cassidy will be the one to talk to regarding that

@wrightmartin
Copy link
Contributor Author

oh, I see the test is failing, although they passed locally - we are now outside of my area of competence

@tmtmtmtm
Copy link
Contributor

tmtmtmtm commented Apr 4, 2018

@wrightmartin it's quite well buried in a wall of text, but I don't think it's your new test that's failing here, but instead it's complaining at line 3632(!):

http://localhost:4567/images/screencast.gif:
2018-04-04 11:43:02 ERROR 404: Not Found.

This is because one of the tests is to essentially try to generate the static site and make sure we're able to do so, but you've removed this image, even though it's still linked from views/house.erb.

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15652 April 5, 2018 08:33 Inactive
@wrightmartin
Copy link
Contributor Author

This is because one of the tests is to essentially try to generate the static site and make sure we're able to do so, but you've removed this image, even though it's still linked from views/house.erb.

Aha, hadn't spotted that, have replaced it with the same as the country template

@tmtmtmtm
Copy link
Contributor

tmtmtmtm commented Apr 5, 2018

Great! Looks like the only thing travis is complaining about now is the awkward spacing in before { get '/iran/' } (NB: running the default rake task — i.e. bundle exec rake — will run not only the tests, but also the rubocop / reek checks etc, which will show you these)

@rich-cassidy
Copy link

@tmtmtmtm @wrightmartin

More fundamentally, though, I'm not convinced that this change makes sense in isolation. It gives a quite confusing message about the project, particularly as it's placed right next to a "How to contribute" link that mentions nothing at all about Wikidata or the Commons, and indeed gives conflicting information.

With regards to this, what accompanying change would be helpful to eliminate / reduce the confusion? It is changing the 'Contribute data' section to mention add info into Wikidata and/or pointing to an appropriate place on Wikidata (perhaps the Wikiproject page)?

@wrightmartin wrightmartin force-pushed the add-democratic-commons-links branch from 4e48c4d to fa4cc51 Compare April 5, 2018 11:36
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15652 April 5, 2018 11:36 Inactive
@tmtmtmtm
Copy link
Contributor

tmtmtmtm commented Apr 6, 2018

@rich-cassidy everypolitician/everypolitician-docs#43 is tracking the related changes that we would need to make to the docs

@tmtmtmtm
Copy link
Contributor

tmtmtmtm commented Apr 6, 2018

All changes made and tests added

Thanks @wrightmartin — I suspect we might need to tweak some of this again before putting it live once the wider issue is resolved, but maybe by then we'll also have accompanying assets too ;)

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.

3 participants