search

tdelmas / website

Let's Encrypt Website and Documentation
https://letsencrypt.org
Mozilla Public License 2.0
2 stars 3 forks source link

Ct logs test2 #32

Closed tdelmas closed 4 years ago

tdelmas commented 4 years ago

This PR #32 and #31 both improve the ct_log shortcode. The current shortcode generate the following html:

<ul>
  <li>explanation...</li>
  <li>explanation...</li>
  <ul>
    <li>log1</li>
    <li>log2</li>
  </ul>
</ul>

which is apparently not valid html (ul can't appear in another ul). It seams to be done on purpose according to the comment on the commit https://github.com/letsencrypt/website/pull/893/commits/76f6afdcbf604485a9097e2249a2cce0b8ee23ec

however, as the markup is preceded by an h3 section, I think it's not necessarily necessary to include the ul in the other ul.

The markup generate by this PR #32 is the following:

<ul>
  <li>explanation...</li>
  <li>explanation...</li>
</ul>
<ul>
  <li style="list-style-type:none;">
  <ul>
    <li>log1</li>
    <li>log2</li>
  </ul>
  </li>
</ul>

The markup of #31 is 

<ul>
  <li>explanation...</li>
  <li>explanation...</li>
  <li style="list-style-type:none;">
  <ul>
    <li>log1</li>
    <li>log2</li>
  </ul>
  </li>
</ul>

Both those markups give the same visual result as the previous one. I believe they also are compatible with screen readers (and they are both preceded by an h3 so the hierarchical situation is clear.) They are also both valid html.

The advantage of #31 is no change in the html content pages. The advantage of #32 is the conversion of the html content pages into markdown, which is easier to translate as it's more coherent with the rest of the documentation.

Both those PR include a simplification of the shortcode.

To compare: Current version: https://letsencrypt.org/docs/ct-logs/ PR #31 : https://deploy-preview-31--letsencrypt-preview-tdelmas.netlify.com/docs/ct-logs/ PR #32 : https://deploy-preview-32--letsencrypt-preview-tdelmas.netlify.com/docs/ct-logs/

tdelmas commented 4 years ago

@pgporada following your awesome PR https://github.com/letsencrypt/website/pull/893 can I have your opinion on this PR (#32) and the PR #31 ?

breakdancingcat commented 4 years ago

@tdelmas and @pgporada I tested #31 and #32 with mac's voice over utility and #31 is read correctly. #32 reads the nested list as an unrelated group of content.

pgporada commented 4 years ago

I like removing $inner = .Inner from the shortcode in #31 and how you cleaned up the html/markdown by replacing data="var" with just "var" in #32. I'm inclined to go with @breakdancingcat because she knows best.

tdelmas commented 4 years ago

@pgporada @breakdancingcat thank you very much both for your feedback!

mac's voice over utility and #31 is read correctly. #32 reads the nested list as an unrelated group of content.

@breakdancingcat just to understand why is #32 incorrect: there is the section (ex. "Production"), then a list of sentences describing that section then the list of logs for that section

why the list of logs doesn't appears related to the section?

Firefox accessibility panel seams to have the correct structure in my opinion: image

Maybe you could share the transcripts of mac's voice over utility so I understand the difference?

Thank you again for your help