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.

Sign up for GitHub

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

Jump to bottom

Code guide: Use short encoding declaration and avoid /> #2069

Merged
merged 7 commits into from
Nov 23, 2021
Merged

Code guide: Use short encoding declaration and avoid /> #2069

merged 7 commits into from
Nov 23, 2021

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Oct 25, 2021 •
edited by pr-preview bot
Loading

I looked into adding a lint for /> with htmlhint, but it seems to not be implemented: htmlhint/HTMLHint#337 (comment)

Preview | Diff

@zcorpan
Copy link
Member Author

zcorpan commented Oct 26, 2021

I looked into adding a lint for /> with htmlhint, but it seems to not be implemented: htmlhint/HTMLHint#337 (comment)

PR for htmlhint to lint for "disallow /> syntax on HTML void elements": htmlhint/HTMLHint#696

@zcorpan zcorpan added the Code Quality Non-functional code changes to satisfy APG code style guidelines and linters label Oct 27, 2021
@zcorpan
Copy link
Member Author

zcorpan commented Oct 29, 2021

The PR for htmlhint was merged. I've updated htmlhint and used the new rule here, so /> syntax is now linted.

@zcorpan zcorpan requested a review from howard-e October 29, 2021 13:50
@zcorpan zcorpan force-pushed the bocoup/code-guide-html branch from 9584c55 to 4977db4 Compare November 1, 2021 14:53
mzgoddard
mzgoddard reviewed Nov 2, 2021
View reviewed changes
package.json Outdated Show resolved Hide resolved
@zcorpan zcorpan mentioned this pull request Nov 2, 2021
Closed
nschonni
nschonni reviewed Nov 2, 2021
View reviewed changes
examples/index.html
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<html lang="en-US"><head>
<meta charset="UTF-8">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in the other files the lowercase "utf-8" is used, but that it doesn't make a functional difference

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I don't care about casing here...

nschonni
nschonni reviewed Nov 2, 2021
View reviewed changes
examples/landmarks/form.html
@@ -143,26 +143,26 @@ <h3>Source Code</h3>
<div class="code">
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an actual code block instead of escaped HTML in a div?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in this PR. 🙂 Feel free to file an issue, though I guess the landmark pages need an overhaul and this would be fixed as part of that when we get to it.

nschonni
nschonni reviewed Nov 2, 2021
View reviewed changes
package-lock.json Outdated
@@ -1,8 +1,10288 @@
{
"name": "aria-practices",
"version": "0.0.0",
"lockfileVersion": 1,
"lockfileVersion": 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Upgrading the lockfile might make sense, but probably better to do it separately

Copy link
Member Author

Choose a reason for hiding this comment

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

#2108

Copy link
Member Author

Choose a reason for hiding this comment

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

That is now merged, so this needs a rebase. cc @howard-e

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@zcorpan zcorpan mentioned this pull request Nov 3, 2021
Open
@zcorpan zcorpan mentioned this pull request Nov 10, 2021
Merged
@zcorpan zcorpan mentioned this pull request Nov 18, 2021
Merged
@howard-e howard-e force-pushed the bocoup/code-guide-html branch from 3e5258f to 868d6a6 Compare November 19, 2021 12:25
zcorpan
zcorpan commented Nov 22, 2021
View reviewed changes
Copy link
Member Author

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

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

🎉

nschonni
nschonni reviewed Nov 23, 2021
View reviewed changes
package.json Outdated
@@ -49,7 +49,7 @@
"eslint-plugin-prettier": "^4.0.0",
"geckodriver": "^1.22.1",
"glob": "^7.1.6",
"htmlhint": "^0.14.2",
"htmlhint": "^0.16.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the package-lock.json changes can be rebased out now

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Think we can land this afterwards

@howard-e howard-e force-pushed the bocoup/code-guide-html branch from c464807 to bc8f78b Compare November 23, 2021 00:47
nschonni
nschonni reviewed Nov 23, 2021
View reviewed changes
package-lock.json Outdated
"version": "2.0.1",
"resolved": "https://registry.npmjs.org/nth-check/-/nth-check-2.0.1.tgz",
"integrity": "sha512-it1vE95zF6dTT9lBsYbxvqh0Soy4SPowchj0UBGj/V6cTPnXXtQOPUbhZ6CmGzAD/rW22LQK6E96pcdJXk4A4w==",
"version": "2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Looked like it reverted some of your latest landed PRs

Copy link
Contributor

Choose a reason for hiding this comment

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

Odd, will take another look at that

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you just need to rebase off the last commit

Copy link
Contributor

@howard-e howard-e Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This should be good now. It seems running npm install currently will revert some references of nth-check and trim-newlines. An npm audit fix should correct the lockfile

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened PR #2158 for that @nschonni

@nschonni
Copy link
Contributor

@howard-e you seem comfortable with rebase, so this git alias might be handy for you https://github.com/nschonni/dotfiles/blob/0095dfa25aabf67d3682a792670e042abba2ec62/.gitconfig#L26. It works on repos, no mater what the default branch name is 😉

@howard-e
Copy link
Contributor

@howard-e you seem comfortable with rebase, so this git alias might be handy for you https://github.com/nschonni/dotfiles/blob/0095dfa25aabf67d3682a792670e042abba2ec62/.gitconfig#L26. It works on repos, no mater what the default branch name is 😉

Appreciate that!

@howard-e howard-e force-pushed the bocoup/code-guide-html branch from bc8f78b to edccb22 Compare November 23, 2021 01:05
@howard-e howard-e mentioned this pull request Nov 23, 2021
Closed
2 tasks
@howard-e howard-e merged commit 858e6ab into main Nov 23, 2021
@howard-e howard-e deleted the bocoup/code-guide-html branch November 23, 2021 17:57
michael-n-cooper pushed a commit that referenced this pull request May 9, 2022
@zcorpan @mzgoddard @howard-e
Code guide: Use short encoding declaration and avoid /> (#2069)
fe6804a 
* Use short encoding declaration and avoid />. Don't include a trailing slash in self-closing elements—the HTML standard says they're optional.
* Use htmlhint 'empty-tag-not-self-closed' rule. This rule checks that HTML void elements do not use '/>' syntax.

Co-authored-by: Michael "Z" Goddard <[email protected]>
Co-authored-by: Howard Edwards <[email protected]>
@alflennik alflennik mentioned this pull request May 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howard-e howard-e howard-e approved these changes

@nschonni nschonni nschonni left review comments

@mzgoddard mzgoddard mzgoddard left review comments

Assignees
No one assigned
Labels
Code Quality Non-functional code changes to satisfy APG code style guidelines and linters
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zcorpan @nschonni @howard-e @mzgoddard