w3c / aria-practices

WAI-ARIA Authoring Practices Guide (APG)
https://www.w3.org/wai/aria/apg/
Other
1.2k stars 332 forks source link

Proposed Code Guide Changes #3060

Open outofambit opened 2 months ago

outofambit commented 2 months ago

Hi, here are some thoughts on changes we could make to the code guide to incorporate newer JS features into them. Normally I would open a PR to propose these, but since it is a wiki page, this seemed like the right place to articulate these first.

I read through the code guide and did some perusing through the current set of examples to get a sense of what the general usage and patterns were and would like to propose adding the following to the JavaScript section of the Code Guide:

jongund commented 1 month ago

I think these are good changes and in general we should be eliminating any thing using aria = aria || {};.

Our biggest issue is having a plan to update legacy examples to the new coding practices.

outofambit commented 1 month ago

@jongund Glad to hear that, thank you for the feedback. I was actually suggesting we standardize on const aria = aria || {}; in each file since all of these examples are at some point running in the context of a larger page and I thought that would be best to have some encapsulation for classes and helper methods. If you have other idea for how to address that I would be very interested to hear them!

css-meeting-bot commented 1 month ago

The ARIA Authoring Practices (APG) Task Force just discussed Code guide proposals.

The full IRC log of that discussion <jugglinmike> Topic: Code guide proposals
<jugglinmike> github: https://github.com/w3c/aria-practices/issues/3060
<jugglinmike> Matt_King: I would like to get some people with the right engineering background to review and comment on the proposed changes
<jugglinmike> Matt_King: I know jongund has already commented
<jugglinmike> Matt_King: Our code has two goals. One: to use modern coding practices to keep up-to-date with what people expects, and two: to write in such a way to make the accessibility motivations clear
<jugglinmike> OliverH: This reminds me of the coding practices on StackOverflow
<jugglinmike> jongund: I'm not so sure about the recommendation to use class fields. I prefer the explicitness of using the "this" keyword, but maybe I'm just set in my way
<jugglinmike> OliverH: I think it's about reducing redundancy
<jugglinmike> Matt_King: Sometimes, a little redundancy is helpful because it can be taxing to discern the relevant context from the surrounding code
<jugglinmike> Matt_King: But then again, I'm not writing JavaScript enough to really weigh in, here
<jugglinmike> OliverH: I can research what the recommended way to do this and get back to you next week
<jugglinmike> arigilmore: I agree with what's been said in the issue, leaning more toward what OliverH has been saying. I'll keep an eye on the issue and share my perspective as necessary
<jugglinmike> Matt_King: Great, then we'll keep this on the agenda for next week
OliverHabersetzer commented 1 month ago

Thanks for posting the log in here - I'm currently looking into the official best practices. I'll post my updates in here and if you want I'll present them in the meeting next week!

OliverHabersetzer commented 1 month ago

Here are some of my findings:


"Use let and const instead of var"

Recommendation: ✅ I agree that we should replace / correct snippets of the old style. Using var is ignorant of the consequences and dangerous in many if not most cases.

Relevant code guide sections:


"Use for...of instead of for loops that don't rely on an index for their logic"

Recommendation: ✅ Personally I'd generally recommend using the newer for...of / for...in variants as they are less error prone to off-by-ones for example, especially when just writing example code without extensive testing.

Relevant code guide sections:


"Update class syntax to use class fields (as in, replace this.foo; with foo; in class declarations)"

Recommendation: ❌ The code guide directly comments on this: Class methods should use this or be made into a static method. We should either follow the guide completely or look for another one. Although I agree that removing this from the examples would reduce redundancy and give our customers less to read, using var or leaving out the this keyword may lead to name clashes that can go unnoticed - so it's probably best to stick with the guide's recommendations.

Relevant code guide sections:


"Use static methods and blocks for shared utilities specific to an example."

Recommendation: ✅ The code guide does not contain this topic - but in my opinion it's a good idea to use static init blocks (I'd call them static constructors, but that might just be me, as my background is in C++) so every variable is scoped to the class / type it belongs to and doesn't clutter the global namespace. In that regard it's similar to the var vs. let debate.


"Standardize on using a global object when creating new functions and classes. A lot of examples do this but some do not and it is not called out in the code guide. (For example, use const aria = aria || {};)"

Recommendation: I don't really have an opinion on this - I'd like to see some examples first. @outofambit can you share one or two maybe so I can see what exactly you would like to change?


PS: I hope this post is to your liking. As I'm still new to the group I tried to make sure to go into as much detail as seemed reasonable, but feel free to critique me 😉

css-meeting-bot commented 1 month ago

The ARIA Authoring Practices (APG) Task Force just discussed Code guide updates.

The full IRC log of that discussion <jugglinmike> Topic: Code guide updates
<jugglinmike> OliverH: I reviewed the code guides that we referenced at our previous meeting
<jugglinmike> OliverH: I tried to match the assumptions in the thread with the content in those guides, and I gave recommendations for each
<jugglinmike> github: https://github.com/w3c/aria-practices/issues/3060
<jugglinmike> Matt_King: I think I would like to move the content of the guide guide out of the wiki because we can't submit pull requests against the wiki
<jugglinmike> Matt_King: We do have a place in the APG for information like this
<jongund> https://github.com/w3c/aria-practices/wiki/Code-Guide
<jugglinmike> Matt_King: If I created a "shell" page for this, would you be willing to draft a patch that relocates the content?
<jugglinmike> OliverH: Sure
<jugglinmike> OliverH: I'll plan to submit this with one commit that simply relocates the information and a separate commit that proposes changes so that it's easier to understand the modifications we're considering
<jugglinmike> Matt_King: That sounds great!