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

FINAL: SC2-4-2-title #116

Merged
merged 7 commits into from May 29, 2018
Merged

FINAL: SC2-4-2-title #116

merged 7 commits into from May 29, 2018

Conversation

jeeyyy
Copy link
Collaborator

@jeeyyy jeeyyy commented May 1, 2018

fixes:

@jeeyyy jeeyyy changed the title FINAL: sc FINAL: SC2-4-2-title May 1, 2018
@jeeyyy jeeyyy mentioned this pull request May 1, 2018
@WilcoFiers
Copy link
Member

+jen as reviewer

carlosapaduarte
carlosapaduarte previously approved these changes May 7, 2018
Copy link
Member

@carlosapaduarte carlosapaduarte left a comment

Choose a reason for hiding this comment

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

The “non-empty text” link, links to a “404 page not-found”

Check if you want the changes to other files unrelated to this rule (especially the one on SC 3.1.1) be part of this pull request. I don’t have any issues with that, but I’m not sure what is the correct procedure.

ShadowBB
ShadowBB previously approved these changes May 7, 2018
<iframe src="../test-assets/sc2-4-2-title-page-without-title.html"></iframe>
</html>
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add a test case where there is no title in the head but only in the body.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added test case. Please review again.

Copy link
Contributor

@kasperisager kasperisager left a comment

Choose a reason for hiding this comment

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

The test case that @JenniferChadwick mentioned would be a good addition.

@jeeyyy
Copy link
Collaborator Author

jeeyyy commented May 14, 2018

kasperisager
kasperisager previously approved these changes May 14, 2018

```html
<html>
<title> <!-- this page has an empty title --> </title>
Copy link
Member

Choose a reason for hiding this comment

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

We need another test case where the first title is empty and the second isn't.

@WilcoFiers
Copy link
Member

Should the rule ID be page-has-title?

@jeeyyy jeeyyy dismissed stale reviews from JenniferChadwick and kasperisager via 1574efa May 22, 2018 12:36
@jeeyyy
Copy link
Collaborator Author

jeeyyy commented May 22, 2018

Have made necessary amends. Please review again.

</body>
</html>
```

Copy link
Contributor

Choose a reason for hiding this comment

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

This test will currently fail the rule, not pass:

The first title element contains non-empty text.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kasperisager - Good spot. Amended.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, guys. Approving.

@jeeyyy jeeyyy merged commit 7b8048d into master May 29, 2018
@jeeyyy jeeyyy deleted the rule-SC2-4-2-title branch May 29, 2018 08:50
@jeeyyy jeeyyy added this to Completed in Rules Progress Overview Jun 11, 2018
@annethyme annethyme moved this from Completed to Auto-Wcag (Pre Publish Review) in Rules Progress Overview Jun 12, 2018
@annethyme annethyme moved this from Auto-Wcag (Pre Publish Review) to Completed in Rules Progress Overview Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants