-
Notifications
You must be signed in to change notification settings - Fork 17
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
Updated app.css to take out the FB login #874
base: integration
Are you sure you want to change the base?
Conversation
@mariobehling pls have a look at the above PR, see if it solves the issue? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide a test deployment link.
@mariobehling github pages build is failing as it seems there are many internal referrals to some non-existent files... (a screenshot attached below) |
@mariobehling can you pls guide? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dishebh Thanks for requesting my review.
My understanding is this will hide login with FB and I'm confident this will work. Hence an approval from my side.
That said, this will increase technical debt, as it just hides the form via CSS. If you plan to remove login with FB permanently I would recommend to remove the markup here https://github.com/voicerepublic/voicerepublic_dev/blob/integration/app/views/devise/sessions/new.html.haml#L21 instead.
When it comes to a "link for testing", I have to ask @mariobehling: Do you have a "staging" system? Without a staging system to deploy testable releases to there is no easy way to provide a "link for testing".
A "fullstack" application like VR cannot be deployed to Github Pages.
I'll be happy to help anyone in the process of setting up a staging system and explain developers how to deploy to it.
Updated the app.css to remove login via FB portal.
Issue #870