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

Proposed Code Guide Changes #3060

Open
outofambit opened this issue Jul 1, 2024 · 6 comments · May be fixed by #3092
Open

Proposed Code Guide Changes #3060

outofambit opened this issue Jul 1, 2024 · 6 comments · May be fixed by #3092
Assignees
Labels
Feedback Issue raised by or for collecting input from people outside APG task force Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation

Comments

@outofambit
Copy link

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:

  • Use let and const instead of var
  • Use for...of instead of for loops that don't rely on an index for their logic
  • Update class syntax to use class fields (as in, replace this.foo; with foo; in class declarations)
  • Use static methods and blocks for shared utilities specific to an example.
  • 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 || {};)
@jongund
Copy link
Contributor

jongund commented Jul 23, 2024

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
Copy link
Author

@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
Copy link
Member

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//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

@mcking65 mcking65 added Feedback Issue raised by or for collecting input from people outside APG task force Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation labels Jul 30, 2024
@OliverHabersetzer
Copy link

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
Copy link

OliverHabersetzer commented Aug 1, 2024

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:

  • clases & constructors

    "Class methods should use this or be made into a static method unless an external library or framework requires using specific non-static methods. Being an instance method should indicate that it behaves differently based on properties of the receiver. eslint: class-methods-use-this"
    Quote from 9.7


"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 😉

@mcking65 mcking65 assigned mcking65 and unassigned howard-e Aug 6, 2024
@css-meeting-bot
Copy link
Member

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//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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feedback Issue raised by or for collecting input from people outside APG task force Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants