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.

Sign up for GitHub

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

Jump to bottom

follow button started working #4453

Merged
merged 4 commits into from Jan 4, 2019
Merged

follow button started working #4453

merged 4 commits into from Jan 4, 2019

Conversation

Rishabh-Kumar-Bothra
Copy link
Contributor

@Rishabh-Kumar-Bothra Rishabh-Kumar-Bothra commented Dec 29, 2018 •
edited by grvsachdeva

Fixes #4443

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 Dec 29, 2018 •
edited

1 Warning
⚠️ It looks like you merged from master in this pull request. Please rebase to get rid of the merge commits – you may want to rewind the master branch and rebase instead of merging in from master, which can cause problems when accepting new code!
1 Message
📖 @geekychaser 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.

Generated by 🚫 Danger

@IshaGupta18
Copy link
Collaborator

@geekychaser have you changes the class of follow button ie. changed the icon from user-plus to eye, to get it working?

oorjitchowdhary
oorjitchowdhary reviewed Dec 30, 2018
View reviewed changes
Copy link
Member

@oorjitchowdhary oorjitchowdhary left a comment

Choose a reason for hiding this comment

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

Please don't change the 'i class' of the follow button.. Let it be 'fa fa-user plus'.. This was actualy an issue to change the icon of the follow buttons

@IshaGupta18
Copy link
Collaborator

@geekychaser you can keep the icon class as it is and just remove the rel="toottip" title="follow" from the original code, because rel="tooltip" was overriding rel="popover", that's why this button wasn't working. If you want to keep the tooltip as well, then you can change rel="popover" to data-toggle="popover".

@Rishabh-Kumar-Bothra
Copy link
Contributor Author

@IshaGupta18 @oorjitchowdhary so the issue was with rel="tooltip" and i removed that and it was working fine.
Reason behind the changing fa-user to fa-eye as in production it seems to be fa-eyes.

@jywarren @gauravano should i change it back to fa-user or let it be as fa-eyes ?

thank you

@oorjitchowdhary
Copy link
Member

Yes please change it to fa fa-user plus..
Refer #4278

@Rishabh-Kumar-Bothra
Copy link
Contributor Author

@oorjitchowdhary yeah okay got it

@Rishabh-Kumar-Bothra
Copy link
Contributor Author

image

i guess this looks fine now ?

@Rishabh-Kumar-Bothra Rishabh-Kumar-Bothra mentioned this pull request Dec 30, 2018
Closed
20 tasks
@grvsachdeva
Copy link
Member

Hi @geekychaser fa-user looks fine. Reviewing the PR.

grvsachdeva
grvsachdeva requested changes Jan 3, 2019
View reviewed changes
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.

Restore the unnecessary link removal, don't use extra space and use proper indentation. Thanks!

app/views/like/_like.html.erb Outdated Show resolved Hide resolved
@oorjitchowdhary oorjitchowdhary mentioned this pull request Jan 4, 2019
Merged
5 tasks
@SidharthBansal
Copy link
Member

There are conflicts please remove them.

@Rishabh-Kumar-Bothra
Copy link
Contributor Author

@jywarren @gauravano please review.
thank you

grvsachdeva
grvsachdeva approved these changes Jan 4, 2019
View reviewed changes
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.

Nice work @geekychaser 💯 !

@grvsachdeva grvsachdeva merged commit 37403dc into publiclab:master Jan 4, 2019
@grvsachdeva
Copy link
Member

grvsachdeva commented Jan 4, 2019 •
edited

Merged 🎉 🎈 ! Thank you!

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
@Rishabh-Kumar-Bothra @grvsachdeva
Follow button working (publiclab#4453)
84e2cb8 
* follow button started working

* fa-eye converted to fa-user-plus

* user link restored
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oorjitchowdhary oorjitchowdhary oorjitchowdhary left review comments

@grvsachdeva grvsachdeva grvsachdeva approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Rishabh-Kumar-Bothra @plotsbot @IshaGupta18 @oorjitchowdhary @grvsachdeva @SidharthBansal