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

Fixes #1397 #1400

Merged
merged 1 commit into from
Apr 27, 2018
Merged

Fixes #1397 #1400

merged 1 commit into from
Apr 27, 2018

Conversation

vyruss
Copy link
Contributor

@vyruss vyruss commented Apr 20, 2018

Fixes #1397 .

Changes proposed in this PR:

  • after_sign_in|up : Check for paths not urls (so that it works on localhost/http)
  • remove duplicate IDs from sign_up page by not including it in modal too (related to Duplicate DOM ids for home page #1064)

@vyruss vyruss self-assigned this Apr 20, 2018
* after_sign_in|up : Check for paths not urls (so that it works on localhost/http)
* remove duplicate IDs from sign_up page by not including it in modal too (related to DMPRoadmap#1064)
Copy link
Contributor

@briri briri left a comment

Choose a reason for hiding this comment

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

The changes to the controller look fine.

I think the changes to the access_controls view though will only partially resolve the issue with multiple form elements with the same id. I think there would still be duplicate email and password elements due to the presence of both the sign in and create account forms.

Feel free to merge, just check the duplicate element stuff before closing that ticket out

@jollopre
Copy link
Contributor

@briri it should not be any duplicate id for the new_user_registration_path view since it has:

  • the sign up form within the main view and
  • the sign in alone in the popup

Note, the home page has loaded both forms too through the tab and there are no duplicated ids there since we covered that on one of the issues from the past. Does it make sense?

@jollopre jollopre merged commit e889f9f into DMPRoadmap:master Apr 27, 2018
@jollopre
Copy link
Contributor

Just merged this but noticed later this goes to master. Do we want to release a version for this small change, or we just wait until we have something else ready?

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.

3 participants