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

Use local index variables for loops #1370

Merged
merged 7 commits into from
Jun 2, 2020
Merged

Use local index variables for loops #1370

merged 7 commits into from
Jun 2, 2020

Conversation

pepelsbey
Copy link
Contributor

Here’s my story:

I took tabs.js file from the tabs implementation and it worked just fine… Until I transpiled it with babel/preset-env and minified with terser. This is a typical thing to do with any modern JS code.

"use strict";!function(){var e,t,n=document…

Babel adds "use strict"; by default and this is what broke it for me. Turns out there are for loops using global index variables:

for (i = 0; i < tabs.length; ++i) {
    addListeners(i);
}

Which works fine until you add "use strict";.

I went through all the sample JS code and added var in front of every index variable (not defined previously) to make it compatible with the strict mode.

Also there are some trailing spaces are gone according to your EditorConfig, sorry about that. I could probably revert that and keep only var in this PR.

@pepelsbey pepelsbey changed the title Use local index variables in for loops Use local index variables for loops Apr 10, 2020
Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

The file: common/script/aria.js
is from the w3c/aria-common repo.

If you'd like to suggest changes for that file, please submit a PR there. However, that file should not have any effect on anything you are doing with APG.

Please revert the changes to this file.

@pepelsbey pepelsbey requested a review from mcking65 April 14, 2020 07:42
@pepelsbey
Copy link
Contributor Author

Done: reverted aria.js

@zcorpan
Copy link
Member

zcorpan commented Apr 17, 2020

I wonder if we should add "use strict" everywhere, and enforce it with a lint check. I can report an issue to discuss it.

@zcorpan
Copy link
Member

zcorpan commented Apr 17, 2020

#1379

@zcorpan
Copy link
Member

zcorpan commented Apr 17, 2020

Also there are some trailing spaces are gone according to your EditorConfig, sorry about that.

I think that's OK, and I wonder why that happens. I suppose some contributors use editors that don't apply the EditorConfig. Maybe this should be enforced with a lint check, too? #1380

@mcking65
Copy link
Contributor

@zcorpan, I see your approval so I assume this is good to go now?

@jongund, just a heads up that we'll be merging this, and it will have a very small effect on 2 of your PRs.

@zcorpan
Copy link
Member

zcorpan commented May 28, 2020

@mcking65 Yes, this is ok to merge.

Copy link
Contributor

@smhigley smhigley left a comment

Choose a reason for hiding this comment

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

There's one more place where the changes re-declare a variable. With that fixed, this looks good to me.

examples/landmarks/js/visua11y.js Show resolved Hide resolved
@mcking65 mcking65 merged commit 81ab634 into w3c:master Jun 2, 2020
michael-n-cooper pushed a commit that referenced this pull request Jun 2, 2020
Multiple Examples Code Quality: Use local index variables for loops (pull #1370)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants