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

Document invalid HTML in date pattern. #556

Merged
merged 4 commits into from Oct 9, 2017

Conversation

Projects
None yet
7 participants
@hannalaakso
Member

hannalaakso commented Sep 25, 2017

This documents the reason for invalid HTML in the date pattern:
A HTML validator will produce the following error for this date pattern: "Attribute pattern is only allowed when the input type is email, password, search, tel, text, or url." This is a known issue. The pattern attribute is needed to force numeric keypads on iOS devices. Type=number alone does not force the numeric keypad. Take a look at this article which explains the issue in more detail.

@selfthinker: do you know if this has been reported to W3?

Fixes #551

Tested in Chrome/FF/Safari.

What type of change is it?

  • Added an explanation to a view using existing elements.
  • I have read the CONTRIBUTING document.

@hannalaakso hannalaakso self-assigned this Sep 25, 2017

@hannalaakso hannalaakso requested review from selfthinker and gemmaleigh Sep 25, 2017

@hannalaakso hannalaakso changed the title from Document invalid HTML in date pattern. Fixes #551 to Document invalid HTML in date pattern. Sep 25, 2017

@edwardhorsford

This comment has been minimized.

Contributor

edwardhorsford commented Sep 25, 2017

Thanks for adding this @hannalaakso. I wonder if we could say it in a more concise way? maybe have Amy / Ellen look at it?

@hannalaakso

This comment has been minimized.

Member

hannalaakso commented Sep 25, 2017

Thanks Ed, I'll ask them to review it.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-elements-review-pr-556 Sep 25, 2017 Inactive

@selfthinker

This comment has been minimized.

Member

selfthinker commented Sep 25, 2017

I've tried re-writing it. What do you think about this?

Please note, this pattern intentionally includes invalid HTML. The pattern attribute is not allowed on number inputs, but it is needed to force numeric keypads on iOS devices.

Also, I think using the panel style doesn't fit here. Or at least I'm not aware we're using it in any similar situation. The problem with the page is that it is quite long and the text might be overlooked. Maybe it's worth putting it at the top above the first example (without the panel style, just a normal paragraph)?

@selfthinker

This comment has been minimized.

Member

selfthinker commented Sep 25, 2017

Regarding the question if this has been reported to the W3C, I don't think there is anything wrong with it. Which kind of patterns would you want to have on a number for which you cannot use max, min and/or step and which are true numbers?
In our case you wouldn't need the pattern if Safari wasn't buggy.

@hannalaakso

This comment has been minimized.

Member

hannalaakso commented Sep 27, 2017

I've rewritten the content with Ellen and @36degrees. This style/format could hopefully be used across elements for documenting similar issues.

@selfthinker Re: the panel style, I'm not too offended by it by I know what you mean. There doesn't seem to be any other elements available for this. @edwardhorsford do you think we should take this to a designer to see if we require a new style for this type of notification?

@hannalaakso hannalaakso requested review from edwardhorsford and removed request for gemmaleigh Sep 27, 2017

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-elements-review-pr-556 Sep 28, 2017 Inactive

@selfthinker

This comment has been minimized.

Member

selfthinker commented Sep 28, 2017

I don't like that it's written like it's blaming validators. If validators tell us about the invalid HTML or not shouldn't be important. What's important is that it's invalid HTML.

As for the panel style, I guess it's best a designer looks at that.

<p>
<a href="http://bradfrost.com/blog/mobile/better-numerical-inputs-for-mobile-forms/" target="_blank">Take a look at this article</a> which explains the issue in more detail.
Note: validators will identify the <span class="bold">pattern</span> attribute as not being valid for inputs
where the <span class="bold">type</span> attribute is <i>number</i>. It is added to <a href="http://bradfrost.com/blog/mobile/better-numerical-inputs-for-mobile-forms" target="_blank">force numeric keypads on iOS devices</a>.

This comment has been minimized.

@selfthinker

selfthinker Sep 28, 2017

Member

Why do you use <span class="bold"> and <i>? We usually use <code class="code"> for these things.

This comment has been minimized.

@selfthinker

selfthinker Sep 28, 2017

Member

We usually never use target="_blank" (except in rare cases) as it is not very user-friendly. I would have added a link to the guidance, but I cannot find it right now. @edwardhorsford, do we have something official anywhere?

This comment has been minimized.

@robinwhittleton
@hannalaakso

This comment has been minimized.

Member

hannalaakso commented Oct 6, 2017

Thanks @selfthinker. I have amended the code tags, removed target="_blank" and amended the note content together with a content designer.

@dashouse: How do you feel us about using the panel element for the note about code here? Is there another element better suited for this?

@hannalaakso hannalaakso requested a review from dashouse Oct 6, 2017

@dashouse

This comment has been minimized.

dashouse commented Oct 6, 2017

Hi, I think that's a valid use of inset text, however it should be inside the column-two-thirds div as it's currently sitting outside of the main layout.

@36degrees

This comment has been minimized.

Contributor

36degrees commented Oct 9, 2017

@hannalaakso it'd be good to remove that merge from the master branch to keep the history simpler. Let me know if I can help with that.

@hannalaakso hannalaakso force-pushed the document-date-pattern-invalid-html branch from 27d13e3 to b3c2647 Oct 9, 2017

@hannalaakso

This comment has been minimized.

Member

hannalaakso commented Oct 9, 2017

Thanks @36degrees that's now done.

Thanks @dashouse I've corrected the column issue, see here

@hannalaakso hannalaakso requested a review from 36degrees Oct 9, 2017

@36degrees

LGTM 👍

@hannalaakso hannalaakso merged commit bdde482 into master Oct 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hannalaakso hannalaakso deleted the document-date-pattern-invalid-html branch Oct 9, 2017

@selfthinker

This comment has been minimized.

Member

selfthinker commented Oct 10, 2017

I don't think that is what @dashouse meant with the column-two-thirds div. It should have been inside the div that was already there, not in a new one.

In the future, I would suggest to merge commits into logical steps. In this case I would have just created 1 commit and not 4 as they are all about the same change and the subsequent commits are about changing the existing change and not adding anything new. Keeping commits about one logical change will make it easier and quicker for people to read and follow the git history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment