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

Added feeds links #586

Closed
wants to merge 1 commit into from
Closed

Added feeds links #586

wants to merge 1 commit into from

Conversation

Welquer
Copy link

@Welquer Welquer commented May 10, 2019

Fixes #434 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@welcome
Copy link

welcome bot commented May 10, 2019

Thanks for opening this pull request! Dangerbot will test out your code and reply in a bit with some pointers and requests.
There may be some errors, but don't worry! We're here to help! 👍🎉😄

@codeclimate
Copy link

codeclimate bot commented May 10, 2019

Code Climate has analyzed commit 8e604bc and detected 0 issues on this pull request.

View more on Code Climate.

@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #586 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #586   +/-   ##
=======================================
  Coverage   71.45%   71.45%           
=======================================
  Files          32       32           
  Lines        1275     1275           
=======================================
  Hits          911      911           
  Misses        364      364

@alaxalves alaxalves mentioned this pull request May 15, 2019
5 tasks
@alaxalves
Copy link
Member

alaxalves commented May 15, 2019

@Welquer First of all, thank you for taking time to contribute, I have made some considerations in #606 that would apply here too. :D Maybe you should check there.

@Welquer
Copy link
Author

Welquer commented May 17, 2019

Thank you too @alaxalves. I also don't like the idea of having a long line, mainly because it interrupts your vertical reading, so you need to horizontally scroll. As it was my first PR on an open source project (🎉), I did as required on the description (just copy and paste), that's why it is as it is. Adding my 2 cents, I'd break it as follows (readability is better):

MapKnitter uses GDALLeafletRuby on RailsImageMagick,  and other open source software, and makes extensive use of the open source font  Junction.

What do you think about it?

@alaxalves
Copy link
Member

Thank you too @alaxalves. I also don't like the idea of having a long line, mainly because it interrupts your vertical reading, so you need to horizontally scroll. As it was my first PR on an open source project (), I did as required on the description (just copy and paste), that's why it is as it is. Adding my 2 cents, I'd break it as follows (readability is better):

MapKnitter uses GDALLeafletRuby on RailsImageMagick,  and other open source software, and makes extensive use of the open source font  Junction.

What do you think about it?

I like it that way, what do you think @jywarren ?

@sashadev-sky
Copy link
Member

sashadev-sky commented Jun 13, 2019

@Welquer Thank you for the PR and sorry for the delay here! There has been a lot going on with mapknitter this month!!

One quick tip about your PR: you need to reference the original opened issue to make sure its linked there and we notice your PR. do this by replacing fixes #0000 with the corresponding issue #. (I updated this one for you).

For your PR, it will need a rebase since there have been merged updates to the file since you last pulled it. Let me know if you need help on this!

As for the code, the extended line doesn't bother me so much but @alaxalves is right that the lines should not be so long and it's a good thing to enforce. With FTOs it is just a copy paste situation so dw! But if you could split the line and push another commit that'd be great! Thanks :)

@cesswairimu
Copy link
Collaborator

This PR has been open for a long time and no activity/ reviews requested has not been addressed. We understand you might be busy and engaged on other things. I am closing this for now...please feel free to reopen if you get some time and would like to finish this. Rem to check if its still available before you reopen. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add RSS feed links to the page footer
4 participants