-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Fix Guide HTML validation #12667
Conversation
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Squashed to one commit. |
@@ -1,4 +1,4 @@ | |||
Creating and Customizing Rails Generators & Templates | |||
Creating and Customizing Rails Generators and Templates |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 &
;, but something along the pipeline is eating it and turning it back into a "&". Probably Nokogiri. I'll look into it.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This seems like a positive change, any reason it hasn't been reviewed again? @senny? |
Willing to rebase against any changes. LMK. |
@nateberkopec please do rebase, I'll see that we can move this forward. |
I had wanted to run this locally and verify the changes, but never got around to it. Sorry about that! |
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. |
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? |
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 @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:
Thanks for your patch by the way! :-) |
[ci skip]
Thanks to @robin850's suggestion, I've changed this PR to update the guides to valid HTML5. Here's a gist of my clean @senny @vijaydev The 502's are just EDIT: Rebased and ready to merge. |
@nateberkopec looking good! Thanks 💛 |
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 withWARNINGS=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 andrake 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!