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

Change <ol> to <ul> when no order is intended (non-normative) #3518

Merged
merged 7 commits into from May 14, 2024

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Oct 24, 2023

The understanding documents are oddly inconsistent when it comes to use of <ol> and <ul>. This edits all cases (particularly in the newer 2.2 understanding docs, but some older ones too) where a list does not have an explicit order and should just be an unordered list


Preview | Diff

The understanding documents are oddly inconsistent when it comes to use of `<ol>` and `<ul>`. This edits all cases (particularly in the newer 2.2 understanding docs, but some older ones too) where a list does not have an explicit order and should just be an unordered list
@bruce-usab
Copy link
Contributor

Briefly discuss on call 3/22/2024, and we determined needed closer review.

@patrickhlauke might this be expedited by spitting the non-normative documents into their own PR?

Also, it is sometimes helpful to have LI numbered if only because that makes them easier to reference in a conversation. Is that enough justification to assert that order matters?

@patrickhlauke
Copy link
Member Author

I guess I could split this PR into two for the changes in the guidelines folder (normative) and the ones in the conformance-challenges and understanding (informative)

I admit I don't buy the rationale about numbered list items as reference, at least not in the cases that I patch here.

@patrickhlauke
Copy link
Member Author

@bruce-usab I've removed the changes to normative documents (I'm assuming conformance-challenges is non-normative?) which would make this less controversial. Filing the same change to the normative ones as a separate PR

@patrickhlauke patrickhlauke changed the title Change <ol> to <ul> when no order is intended Change <ol> to <ul> when no order is intended (non-normative) Mar 23, 2024
patrickhlauke added a commit that referenced this pull request Mar 23, 2024
@patrickhlauke
Copy link
Member Author

Done #3756

Copy link
Contributor

@bruce-usab bruce-usab left a comment

Choose a reason for hiding this comment

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

Thank you @patrickhlauke for splitting off the normative docs. I had hoped to get to this PR on the call today, but I messed up the opportunity for that.

Everything looks good to me, but I think the conformance-challenges file has hard-coded references to 2.1 instead of 2.2. I think that might be addressed in a different PR in the future, or maybe split off the one file to get the other 13 quickly through?

I retract my comment about maybe OL having some utility.

  • Some of the LI are sorted by SC numbering, so OL is unhelpful.
  • Other places OL is clearly semantically correct, since there is not any apparent order. I can't think of situations where having a number to refer to would matter. The lists are short and it's better to refer to the thing than the number for the thing.

@fstrr fstrr self-assigned this Apr 5, 2024
@fstrr fstrr self-requested a review April 5, 2024 17:26
@fstrr
Copy link
Contributor

fstrr commented Apr 5, 2024

@patrickhlauke There's a couple of merge conflicts that have appeared. The Conformance Challenges one is straight forward, but the Text Spacing one looks like there's more to it than that and is conflicting with #3572, which was merged into main three days ago. Could you take a look at that one please?

@patrickhlauke
Copy link
Member Author

@fstrr thanks for checking. sigh...

@fstrr
Copy link
Contributor

fstrr commented Apr 5, 2024

@patrickhlauke Could be worse

@patrickhlauke
Copy link
Member Author

@fstrr sorted (the text spacing one was odd ... it was showing a huge chunk being out of sync, but in reality it wasn't)

@fstrr
Copy link
Contributor

fstrr commented Apr 5, 2024

@patrickhlauke Oh, that's good.

@patrickhlauke
Copy link
Member Author

@fstrr want me to have a look at the mega-conflicts in the other one? I suspect it's something small, like a space before a closing /, but in lots of files, and in a way that confuses github's merge capability...

@fstrr
Copy link
Contributor

fstrr commented Apr 5, 2024

@patrickhlauke If you want to, that would be super helpful. It looks like your "chore: update HTML files for xslt/xml processing" update is the issue with the 66 files, so, yeah, it's probably something really straightforward. Thanks :)

Copy link
Contributor

@bruce-usab bruce-usab left a comment

Choose a reason for hiding this comment

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

I have reviewed changes since I last reviewed, and I have no concerns.

@alastc
Copy link
Contributor

alastc commented Apr 12, 2024

@patrickhlauke to check that, once built, the techniques still come out (e.g. for focus-not-obscured-min).

The XSLT may forget to output them if it's not expecting <ul>s.

@bruce-usab
Copy link
Contributor

bruce-usab commented Apr 12, 2024

This PR includes the one source file for Challenges with Accessibility Guidelines Conformance and Testing, and Approaches for Mitigating Them. I think that is fine, since (1) it is an AG WG document, which (2) is a working draft, and (3) this is editorial. Per the discussion on the call 4/12, @mbgower this detail might be worth flagging when sent for WG review.

@mbgower mbgower merged commit ce91353 into main May 14, 2024
1 check passed
@mbgower mbgower deleted the patrickhlauke-techniques-lists branch May 14, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants