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

Fix Guide HTML validation #12667

Merged
merged 1 commit into from
May 6, 2014
Merged

Fix Guide HTML validation #12667

merged 1 commit into from
May 6, 2014

Conversation

nateberkopec
Copy link
Contributor

Hi!

The Ruby on Rails Guides Guidelines say that you should make sure all guides are valid HTML and that bundle exec rake guides:generate should be run with WARNINGS=1 to make sure our internal links are valid. To my surprise, there were quite a few validation errors!

Now, rake guides:validate returns no errors and rake guides:generate WARNINGS=1 returns no warnings.

I left these commits small and separate, please instruct if you wish me to squash them.

Expect a PR soon moving the guides to HTML5, hopefully it won't be too much work!

@AJ-Acevedo
Copy link
Contributor

The project prefers that all Pull Requests are squished and you add [ci skip] to the commit message.

@@ -1,6 +1,6 @@
module RailsGuides
class Markdown
class Renderer < Redcarpet::Render::HTML
class Renderer < Redcarpet::Render::XHTML
Copy link
Member

Choose a reason for hiding this comment

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

If we want to be HTML 5 compliant, this change is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - I was aiming for validation with the current, XHTML 1.0 Strict doctype before making any changes for HTML5. This PR is for a validating XHTML 1.0 Strict page, not an HTML5 one. If I switch the doctype to HTML5, I'll make a separate PR.

@nateberkopec
Copy link
Contributor Author

Squashed to one commit.

@@ -1,4 +1,4 @@
Creating and Customizing Rails Generators & Templates
Creating and Customizing Rails Generators and Templates
Copy link
Member

Choose a reason for hiding this comment

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

can't we escape the & instead of using and?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm - I've tried using &amp;, but something along the pipeline is eating it and turning it back into a "&". Probably Nokogiri. I'll look into it.

Copy link
Member

Choose a reason for hiding this comment

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

I see... is this the only & in the guides then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't - but it's the only & that's getting pulled into a title tag, which is what's causing the validation error. And, it looks like we were setting html_safe on the title string, which was causing the title to be inserted without escaping.

html_safe seems unneccesary here, so I've removed it, which causes the & to be escaped. All guides validate correctly.

@nateberkopec
Copy link
Contributor Author

@vijaydev I have removed my deletion of the invalid link ("WEBrick isn't your only option for serving Rails. We'll get to that later").

This PR still returns: All checked guides validate OK! from rake guides:validate

@ghost ghost assigned vijaydev Dec 28, 2013
@JonRowe
Copy link
Contributor

JonRowe commented Apr 30, 2014

This seems like a positive change, any reason it hasn't been reviewed again? @senny?

@nateberkopec
Copy link
Contributor Author

Willing to rebase against any changes. LMK.

@senny
Copy link
Member

senny commented May 1, 2014

@nateberkopec please do rebase, I'll see that we can move this forward.

@vijaydev
Copy link
Member

vijaydev commented May 2, 2014

I had wanted to run this locally and verify the changes, but never got around to it. Sorry about that!
@senny If you are giving this a shot, please do :) Else, I can take this up sometime in the coming week.

@vijaydev
Copy link
Member

vijaydev commented May 2, 2014

Now I realize why I never got to this. I end up getting 502 Proxy Errors whenever I run validation on my box. I'm fine with the changes made and once Nate rebases, we can merge.

@senny
Copy link
Member

senny commented May 4, 2014

Tried to run validations on this branch as well. Got a few "Proxy Errors" but beside that it still printed tons of validation errors. Am I doing it wrong?

https://gist.github.com/senny/0213ea3b4c5c38947ea2

@robin850
Copy link
Member

robin850 commented May 4, 2014

Hi there,

Sorry if I'm missing something but why would we want to be XHTML 1.0 compliant ? I'm pretty sure less work would be needed if we were at making these guides HTML 5 compliant. I'm also wondering if the result provided by the w3c_validators gem are trustworthy since the last release of this gem was almost 3 years ago.

@senny : I guess you've already generated the guides locally so it is validating the old ones. You can run the following but there will still be the proxy errors:

$ bundle exec rake guides:generate guides:validate

Thanks for your patch by the way! :-)

@nateberkopec
Copy link
Contributor Author

Thanks to @robin850's suggestion, I've changed this PR to update the guides to valid HTML5.

Here's a gist of my clean rake guides:validate output.

@senny @vijaydev The 502's are just w3c_validators struggling to POST from time to time. It never really happens twice if you just run it again.

EDIT: Rebased and ready to merge.

@senny
Copy link
Member

senny commented May 6, 2014

@nateberkopec looking good! Thanks 💛

senny added a commit that referenced this pull request May 6, 2014
@senny senny merged commit 8357d3a into rails:master May 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants