Review updates to alert example page

[mcking65]

Review changes to the
Alert Example Page
made in commit 9a30de1.

Reviews requested on Feb 17, 2017

• Review by @MichielBijl
• Review by @jnurthen
• Add the example code generation back in.

[ZoeBijl]

This looks good, I’ve made two changes in 5423bf7:

• Added a line of CSS that hides the alert container when empty
• Manually added the source code to the page; the generated code that was there was more confusing than it was helpful.

[mcking65]

@MichielBijl wrote:

This looks good, I’ve made two changes in 5423bf7 :

Added a line of CSS that hides the alert container when empty

OK, sounds good.

Manually added the source code to the page; the generated code that was there was more confusing than it was helpful.

Hmmmmm? This is part of what I just undid. Relying on the display script is an important aspect of accuracy and maintainability.

If the source code of the example itself is confusing, then I think we need to rework the example or fix the examples/js/example.js.

I did notice that example.js does not include html comments and left out the html inside the script template element. But, those are bugs in example.js that should be fixed rather than not using example.js.

That aside, What was confusing about the HTML source?

Please revert.

[ZoeBijl]

@mcking65 this is what I put in the code block:

<button id="alert-trigger">Trigger Alert</button>

<div id="example" role="alert"></div>

<!-- The following script element contains the content that will be inserted into the alert element. --> <script type="text/template" id="alert-template"
  <p>Hej, hello, ciao, こんにちは, 안녕</p
</script>

This is what example.js put in there:

<div id="ex1">
  <p>
    <button id="alert-trigger">
      Trigger Alert
    </button>
  </p>
  <p>
     This is just a test. A typical alert is triggered by an event, such as an error, warning condition, or the arrival of information that is important in the context of the user's task. 
  </p>
  <div id="example" role="alert">
  </div>
  <script type="text/template" id="alert-template">
    
Hej, hello, ciao, こんにちは, 안녕


  </script>
</div>

A couple of things are wrong with this:

• The div="ex1" is included erroneously
• The paragraphs “This is just a test…” has no place in the example code
• The paragraph that wraps the text inside id="alert-tempalte" is gone

While I agree that automatically copying in the code is good for maintenance, it’s no good if the example makes no sense. I won’t pretend that I know how to reliably solve this with JS right now. Maybe you know somebody that is able to do that.

That said, I don’t mind keeping the code as shown on the page and the actual code of the example in sync.

[mcking65]

@MichielBijl wrote:

A couple of things are wrong with this:

The  div="ex1"  is included erroneously

That is the case in all examples, and is easily fixable. I had no idea it was such a distraction. @jongund made the script work that way. So, I raised issue #294.

The paragraphs “This is just a test…” has no place in the example code

I added that explanitory text; I will move it out.

The paragraph that wraps the text inside  id="alert-tempalte"  is gone

Yes, this is also a bug in example.js script. I included it in issue #294.

While I agree that automatically copying in the code is good for maintenance, it’s no good if the example makes no sense.

I would say the example.js output still makes sense; it is just not as clean as we would prefer.

That said, I don’t mind keeping the code as shown on the page and the actual code of the example in sync.

From an editor's perspective, keeping focus on the objectives of the project, It is not a good idea to build inconsistencies like this into our pages. Let's fix the root causes of the problems so we can have consistently well-maintained and high quality content that serves our goals.

I would still like to return to the generated output and get issue #294 resolved.

[ZoeBijl]

Okay, I’ll add the example code generation back in.

[mcking65]

@MichielBijl, friendly ping on todo:
"Add the example code generation back in."

[ZoeBijl]

@mcking65 thanks, slipped my mind during my vacation. I added this back into the example in ef34006.

In addition I moved the paragraph that was in the code example itself to right after the heading level 2 “Example”.

[mcking65]

Thanks @MichielBijl. In commit ef34006, I also see 2 additions and 2 deletions in examples/radio/radio-1/radio-1.html. However, I can't tell the difference between the old and new lines. Is that just some incidental white space change? Or, am I not hearing the difference.

Friendly reminder, in commit titles, please use the example title and in either the title or body of the message, include the issue link. e.g.:

Alert Example: Restore Auto HTML Source Generation for Issue #291

The # prepended to an issue or pr number automatically creates the link.

[ZoeBijl]

Err, crap, I was very careful not to commit those changes, but seems I somehow messed up. I changed it back. The change was in the path to the CSS file.

I’ve added your suggestion for the commit title as a comment on the commit itself.

[pauljadam]

Is it possible to get the alert to remove itself and re speak the alert when you hit the button multiple times so you don't have to reload the page to try the example again? Or maybe even handle multiple types of alerts, e.g. with form validation there may be different alert messages that appear and disappear without the page refreshing and that's often hard to get working correctly with the screen readers.

[mcking65]

@pauljadam, what browser are you using and with which screen reader?

I think VoiceOver may have a bug. You shouldn't have to reload the page to retrigger the announcement. It works correctly with NVDA in Firefox and Chrome and with JAWS in Firefox. JAWS has a bug in Chrome. It only announces once with VoiceOver in Chrome. When using VoiceOver, you have to reload the page.

I do agree with your feedback about offering examples of other ways to trigger and render so I opened issue #327 to enhance the example.

[jnurthen]

Shouldn't the example have lang attributes on it?
<p><span lang="da">Hej</span>, hello, <span lang="it">ciao</span>, <span lang="ja">こんにちは</span>, <span lang="ko">안녕</span></p>

other than that I am fine with it.

[pauljadam]

@mcking65 macOS and Safari latest versions on desktop. I see it does work to speak the alert again on button click with VoiceOver iOS latest so I guess this is a bug with VoiceOver macOS.

At first I thought it was the behavior of the demo because when you hit the trigger alert button when the alert is already visible the alert does not disappear and then re appear, it just stays there always visible each time you click it so I was thinking that the visible alert should disappear and then reappear each time you click it for the visual and audio affect. Thanks!

[ZoeBijl]

@jnurthen it should, and now does, thanks!

@pauljadam I like the idea of having multiple types of alerts. Only issue I see is that it would complicate the example code quite a bit. In that case I’d like to pull the different alerts from a JSON file with a generic HTML template. I think it’s an interesting idea and definitely something to re-visit for APG 1.2. Thoughts @mcking65 @jnurthen?

[mcking65]

Based on completed reviews and discusssion in March 20 meeting, closing as complete.