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

Contact page #323

Merged
merged 0 commits into from
Jul 11, 2016
Merged

Contact page #323

merged 0 commits into from
Jul 11, 2016

Conversation

zarino
Copy link
Member

@zarino zarino commented Jun 20, 2016

Connects to mysociety/alaveteli#74.

To-do:

  • Reject form unless "I understand" checkbox has been checked.
  • Form error handling – validation errors should be displayed in the correct place, the last houdini-input should be "checked" so the form is visible, and the user should be sent down to the #wdtk anchor.
  • Hide "Contact" section of sidebar.
  • Welsh translation.
  • Nicer error message for not checking the box

screen shot 2016-06-20 at 12 31 02

screen shot 2016-06-20 at 12 31 07

screen shot 2016-06-20 at 12 31 16

@lizconlan
Copy link

@zarino - could you check I've not broken your design before I hand this over for a review of the Rails code?

<p>
<label class="form_label" for="contact_name">My name:</label>
<%= f.text_field :name, :size => 20 %>
(or <%= link_to "sign in", signin_url(:r => request.fullpath) %>)
Copy link
Member Author

Choose a reason for hiding this comment

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

@lizconlan – This return path should include the "#wdtk" location fragment, so that when they return to the contact page after logging in, they're automatically taken down to the right section of the page.

@zarino
Copy link
Member Author

zarino commented Jun 30, 2016

@lizconlan – I've added a few commits tidying up the error styles, but other than that, it all looks good!

@zarino zarino assigned lizconlan and unassigned zarino Jun 30, 2016
@lizconlan
Copy link

lizconlan commented Jun 30, 2016

@zarino I got a bit confused (easily done, admittedly) when there were "required" form fields as it just seemed as though the form was broken - I missed the visual cue of the disable submit button and was wondering why nothing was happening when I clicked on it. Although to be fair, I was expecting me to have broken something so...

@lizconlan
Copy link

Have fixed the post signin redirect. But... the client-side validation doesn't work in Safari, it just looks like the form submit's broken.

@zarino
Copy link
Member Author

zarino commented Jun 30, 2016

Sigh. Safari (on Mac and iOS) doesn't show a notification when a required form input is left empty: http://caniuse.com/#search=required (webkit bug from 7 years ago).

It is, however, beneficial for users with more compliant browsers, because it saves the page having to reload before you see your mistake.

Maybe we use JavaScript to remove the "required" attribute on Safari only? Would the form submit then?

@lizconlan
Copy link

7 year old bug in Safari, you say? Wish I could be more surprised. Yeah that should work - the only other thing I can think is to make the disabled submit button more obvious but that's probably harder/more disruptive.

@zarino
Copy link
Member Author

zarino commented Jun 30, 2016

Yes, we should aim to avoid the submit button being disabled at all. If that means we have to remove form validation entirely for all browsers, just so that it works for Safari, then so be it. But it'd be nice if we could just remove it for Safari, and keep it for the more modern browsers.

@lizconlan
Copy link

A bit clunkily done - if I say so myself - but should be Safari-proof now

@lizconlan lizconlan removed their assignment Jun 30, 2016
@lizconlan lizconlan changed the title WIP: Contact page Contact page Jun 30, 2016
@garethrees garethrees self-assigned this Jul 4, 2016
<li>
If you want to make a public Freedom Of Information
request to a UK government department, you can
<a href="<%= new_request_path %>">start here</a>.
Copy link
Member

Choose a reason for hiding this comment

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

Clicking this redirects you to the homepage, You want select_authority_path.

Choose a reason for hiding this comment

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

D'oh! Thanks

Choose a reason for hiding this comment

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

(ah, that's been copied across from the original and is what's currently on the site - good catch @garethrees)

@garethrees
Copy link
Member

Couple of minor bits but after that good to go. Lets handle the Welsh translations as part of mysociety/alaveteli#3137, as there'll be some additional translation to do there.

@garethrees garethrees assigned lizconlan and unassigned garethrees Jul 6, 2016
@garethrees
Copy link
Member

Assigned to @lizconlan to handle minor fixups and merge

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.

5 participants