-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
make Sign Up more obvious #925
Conversation
078b150
to
0d6f9c4
Compare
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.
@hairedfox , this looks great! 💥
Just one minor comment.
@@ -38,6 +38,7 @@ | |||
</div> | |||
<div class="" id="login_button"> | |||
<%= f.button :submit, "Log in", class: "button is-primary" %> | |||
<%= link_to "Sign Up", new_user_registration_path, class: "button is-success" %> |
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.
I think this should probably not live inside the #login_button
div and instead be in a separate sibling div.
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.
Thank you for reminding me. I'll fix this one and merge.
@@ -2,7 +2,7 @@ | |||
<%= link_to "Log in", new_session_path(resource_name) %><br /> | |||
<% end %> | |||
|
|||
<%- if devise_mapping.registerable? && controller_name != 'registrations' %> | |||
<%- if devise_mapping.registerable? && !controller_name.in?(['registrations', 'sessions']) %> |
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.
TIL about in?
! 🙏🏾
0d6f9c4
to
56acab8
Compare
Why
What
How
Testing
Next Steps
Outstanding Questions, Concerns and Other Notes
Accessibility
Security
Meta
Pre-Merge Checklist