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

Removed Extra Space after 'Share' #4612

Merged
merged 2 commits into from
Feb 15, 2019
Merged

Conversation

jaidevshriram
Copy link
Contributor

@jaidevshriram jaidevshriram commented Jan 14, 2019

Fixes Issue : #4611

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

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

  • 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 📁
  • PR is descriptively titled 📑
  • 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!

@plotsbot
Copy link
Collaborator

plotsbot commented Jan 14, 2019

2 Messages
📖 @jaidev123 Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progress’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

@@ -29,8 +29,6 @@

<!-- errors module -->
<div class="ple-module container">
Copy link
Member

Choose a reason for hiding this comment

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

- <div class="ple-module container">

Remove this line too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right. Removing the closing div as well

@@ -29,8 +29,6 @@

<!-- errors module -->
<div class="ple-module container">

<div class="col-md-3"></div>
<div class="ple-errors col-md-9"></div>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div class="ple-errors col-md-9"></div>
<div class="ple-errors"></div>

Do this..

@oorjitchowdhary
Copy link
Member

Hi @jaidev123, welcome to Public Lab!
I've suggested some changes in the code. Please make them.

Also, upload a screenshot of the feature working on your localhost
Thanks

@oorjitchowdhary oorjitchowdhary changed the title Removed Extra Sprace after 'Share' Removed Extra Space after 'Share' Jan 14, 2019
@jaidevshriram
Copy link
Contributor Author

@publiclab/reviewers Hey! Currently, it seems like I have to create a new PR in order to add the suggested changes proposed by @oorjitchowdhary . Is there another way? (instead of closing this PR and then starting another one). Also, how can I run this on my PC?

Thanks!

@oorjitchowdhary
Copy link
Member

Hey, you can still make changes in this PR. You opened this PR from the patch-1 branch of your forked repo.. You can go to that branch (JAIDEV123/plots2:patch-1) and do the changes..

@jaidevshriram
Copy link
Contributor Author

Hey!

Looks like all checks have passed :)

@oorjitchowdhary
Copy link
Member

Hi @jaidev123, this looks great. We just require screenshots now.
You can setup plots2 repo on your localhost through the steps given in README.md
Thanks

@jaidevshriram
Copy link
Contributor Author

Seems to have hit a roadblock.

screenshot 4

I've tried bundle install and bundle update with no progress. Tried rolling back ruby to 2.4 but error message states that I have to use 2.5 or later. Do you know of a fix for this? Could this be because patch-1 branch hasn't been updated in some time?

@jywarren
Copy link
Member

jywarren commented Jan 17, 2019 via email

@jaidevshriram
Copy link
Contributor Author

Hey. It looks like I'm going to be busy for a while. Could someone else please continue this?

@grvsachdeva
Copy link
Member

Hi @jaidev123, no issue. I will add the screenshot on your behalf and complete this PR. Thanks!

@grvsachdeva
Copy link
Member

Post page view:

editor_view

I think, we are not rendering anything inside deleted div, right?
@rexagod @jywarren please give one more review for this to merge. Thanks!

Copy link
Member

@grvsachdeva grvsachdeva left a comment

Choose a reason for hiding this comment

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

Looks good to me 🎉

@jywarren jywarren merged commit 6497938 into publiclab:master Feb 15, 2019
@grvsachdeva
Copy link
Member

Merged 🎉 🎈 !

Hi @jaidev123, thanks for the nice work. We would love to have more help from you. If you are interested in solving more issues then check here - https://code.publiclab.org/#r=all for a new issue. In case, you are unable to find an issue, feel free to comment here. Thanks!

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
* Removed Extra Sprace after 'Share'

Fixes Issue : publiclab#4611

* Update rich.html.erb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants