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

Login modal like wiki #4186

Merged
merged 13 commits into from
Dec 11, 2018
Merged

Login modal like wiki #4186

merged 13 commits into from
Dec 11, 2018

Conversation

okonek
Copy link
Contributor

@okonek okonek commented Dec 8, 2018

Fixes #4154 (<=== 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
  • 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!
wikilikemodal

@okonek
Copy link
Contributor Author

okonek commented Dec 8, 2018

@SidharthBansal I think we can accept it.

@SidharthBansal
Copy link
Member

NO MERGING OF LINKING ISSUES OF MODAL NOW,
we will be merging these after the signup modal is complete.
Just we are exploring things here.
I am reviewing give me some time

@plotsbot
Copy link
Collaborator

plotsbot commented Dec 8, 2018

2 Messages
📖 @okonek 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 progresss’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

@okonek
Copy link
Contributor Author

okonek commented Dec 8, 2018

Ok, thanks!

@SidharthBansal
Copy link
Member

SidharthBansal commented Dec 8, 2018

@okonek please check

  • whether you are able to login in or not.
  • are you redirecting back to the same page after login or not
  • is note liked or not.
  • ensure that login modal does not pop up if the user is already logged in

@okonek
Copy link
Contributor Author

okonek commented Dec 8, 2018

Ok, wait a second.

@okonek
Copy link
Contributor Author

okonek commented Dec 8, 2018

  • whether you are able to login in or not.
  • are you redirecting back to the same page after login or not
  • is note liked or not.
  • ensure that login modal does not pop up if the user is already logged in

@okonek
Copy link
Contributor Author

okonek commented Dec 8, 2018

The redirecting is not implemented in the new modal. Should I do it?

@SidharthBansal
Copy link
Member

SidharthBansal commented Dec 8, 2018 via email

@okonek
Copy link
Contributor Author

okonek commented Dec 8, 2018

@SidharthBansal Ok, so now I added a redirect, but I don't know how to automatically like the wiki after the login and the redirect. Have you got any idea?

@SidharthBansal
Copy link
Member

Can you please provide me the screenshot for the 3 sub-issues which are solved?

@SidharthBansal
Copy link
Member

Or can you push your changes on the unstable so that we can test them?

@@ -1,6 +1,6 @@
<div class="container">

<%= form_for :user_session, :as => :user_session, :url => :user_sessions, :html => {:class => "form col-md-6 offset-3"} do |f| %>
<%= user_sessions_url + "?return_to=" + request.fullpath %>

This comment was marked as resolved.

Copy link
Member

Choose a reason for hiding this comment

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

@jywarren @okonek I have pushed the changes for this return_to on the unstable.
We can test this now.

Copy link
Member

Choose a reason for hiding this comment

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

As this return_to is highly related to @oorjitchowdhary and @JonathanXu1 I have created a pr #4197, @okonek you can remove these changes from your pr.
As @okonek you helped us in return_to, you will get rewards for https://codein.withgoogle.com/dashboard/tasks/5291275109531648/.
Please take it so that I can approve your task.

Copy link
Member

Choose a reason for hiding this comment

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

It is working fine on unstable.

@kevinzluo
Copy link
Collaborator

I might be mistaken, but I believe this also fixes #4153 since Notes and Wikis both use app/views/like/_like.html.erb .

@SidharthBansal
Copy link
Member

Let's first complete return_to field issue present in app/views/user_sessions/_new.html.erb.
Then we can move forward. One piece at a time.

@SidharthBansal
Copy link
Member

@kevinzluo if that's the case then once this pr gets merged we will close both the issues.

@okonek
Copy link
Contributor Author

okonek commented Dec 8, 2018

I removed the file. Is it good now?

@SidharthBansal
Copy link
Member

You just need to remove the changes which you did.
No need to remove the entire file.

@okonek
Copy link
Contributor Author

okonek commented Dec 8, 2018

Ok

@okonek
Copy link
Contributor Author

okonek commented Dec 8, 2018

I added it, is everything looking good?

@SidharthBansal
Copy link
Member

No problem, @gauravano you can potentially check this on unstable and merge this.
We can dismiss the codeclimate.

@okonek
Copy link
Contributor Author

okonek commented Dec 9, 2018

Ok, no problem. I hope he'll do it today, because I wanted to claim the last task before the deadline.

@okonek
Copy link
Contributor Author

okonek commented Dec 9, 2018

@SidharthBansal Could you do it, please? The competition ends tomorrow.

@SidharthBansal
Copy link
Member

SidharthBansal commented Dec 9, 2018

I will approve your both tasks, I believe your changes are right. Please claim them on GCI dashboard and submit for review now. I will approve them now.
I want @gauravano to test because many errors are present which are not detected by the developer but by the user. So he will be testing it as a user. This will be great.

@okonek
Copy link
Contributor Author

okonek commented Dec 9, 2018

Thank you, but I can't take a new task if the old is not approved, so can you approve?

@SidharthBansal
Copy link
Member

SidharthBansal commented Dec 9, 2018 via email

@okonek
Copy link
Contributor Author

okonek commented Dec 9, 2018

I did it, take a look.

@SidharthBansal
Copy link
Member

SidharthBansal commented Dec 9, 2018 via email

@okonek
Copy link
Contributor Author

okonek commented Dec 9, 2018

@SidharthBansal You mean this?
jp2

@SidharthBansal
Copy link
Member

Yeah

@okonek
Copy link
Contributor Author

okonek commented Dec 9, 2018

Ok, claimed and submitted. Thank you very much.

@SidharthBansal
Copy link
Member

SidharthBansal commented Dec 9, 2018 via email

@okonek
Copy link
Contributor Author

okonek commented Dec 9, 2018

Could you link it to me? I can't find it.

Copy link
Member

@SidharthBansal SidharthBansal left a comment

Choose a reason for hiding this comment

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

Hi, changes looks good to me. Thanks for your work.
@jywarren @gauravano can you please review this and merge this?

@SidharthBansal
Copy link
Member

@okonek
Copy link
Contributor Author

okonek commented Dec 9, 2018

@SidharthBansal
Could you accept this task to me? Because my solution to this one, actually also solved that one. Thanks.

jp2

@okonek
Copy link
Contributor Author

okonek commented Dec 9, 2018

@SidharthBansal
Copy link
Member

SidharthBansal commented Dec 9, 2018 via email

@okonek
Copy link
Contributor Author

okonek commented Dec 9, 2018

Thank you so much for help in this tasks.

@SidharthBansal
Copy link
Member

@jywarren @gauravano now I think it can be merged. What you guys think?

@SidharthBansal SidharthBansal merged commit eeed664 into publiclab:master Dec 11, 2018
@ghost ghost removed the ready label Dec 11, 2018
@SidharthBansal SidharthBansal mentioned this pull request Dec 14, 2018
20 tasks
oorjitchowdhary pushed a commit to oorjitchowdhary/plots2 that referenced this pull request Dec 21, 2018
* Login button for the modal fixed

* Clicking on a like button if not logged in now shows login modal

* Added a redirect in login modal

* Removed changes from _new.html.erb

* Delete _new.html.erb

* Create _new.html.erb

* Added custom param option to the login modal and like now auto-likes after login

* Fixed the issue of checking array length on nil

* Fixed codeclimate issues
SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
* Login button for the modal fixed

* Clicking on a like button if not logged in now shows login modal

* Added a redirect in login modal

* Removed changes from _new.html.erb

* Delete _new.html.erb

* Create _new.html.erb

* Added custom param option to the login modal and like now auto-likes after login

* Fixed the issue of checking array length on nil

* Fixed codeclimate issues
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.

None yet

5 participants