Skip to content

Conversation

benesch
Copy link
Contributor

@benesch benesch commented May 30, 2017

See individual commits for details!


This change is Reviewable

@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

Copy link
Contributor

@madelynnblue madelynnblue left a comment

Choose a reason for hiding this comment

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

LGTM, with the caveat that I don't know any ruby.

@cockroach-teamcity
Copy link
Member

@jseldess
Copy link
Contributor

Don't see why this would have anything to do with your changes, but the hubspot email sign-up at the top of each release notes page is missing:

screen shot 2017-05-31 at 12 43 46 pm

Compare that to the live page:

screen shot 2017-05-31 at 12 44 00 pm

@@ -0,0 +1,16 @@
module TemplateWithCaching
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment to explain the purpose of this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up just nuking that module; causing more problems than it solved.

@jseldess
Copy link
Contributor

jseldess commented May 31, 2017

Haven't look through all pages, but so far, I've found these issues. Happy to help fix these.

  • On Install Client Drivers, the html comment is now visible for some reason.
  • On Constraints, some of the code blocks aren't getting indented properly.
  • On Data Types, the main table is broken, and the green tip isn't indented properly.
  • On SERIAL, the number and indentation are off here.
  • More code block and callout indentation issues on COLLATE.
  • On Recommended Production Settings, some anchor links, steps, and code blocks aren't rendering.

@sploiselle
Copy link
Contributor

Railroad diagrams are broken (example)

@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

1 similar comment
@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

@benesch
Copy link
Contributor Author

benesch commented Jun 2, 2017

Ok, I think I've fixed all the bugs you guys found! PTAL.

@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

@benesch benesch force-pushed the fast-compiles branch 2 times, most recently from cf4191e to d369515 Compare June 2, 2017 19:58
@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

@benesch benesch force-pushed the fast-compiles branch 2 times, most recently from 0d438e2 to 8b4fe1c Compare June 2, 2017 20:54
@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

@benesch
Copy link
Contributor Author

benesch commented Jun 2, 2017

This is finally green! 🎉

Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

LGTM, @benesch! Thanks for getting this to work. If @sploiselle also good with this, go ahead and merge.

Sidenote: On other PRs, Team City is failing on missing gems and a bundle file. Is that related to this PR. Will we just have to rebase those PRs once this is merged?

@benesch
Copy link
Contributor Author

benesch commented Jun 5, 2017

Eek, yep, my fault. Let me see if I can appease TeamCity so it works with and without this PR.

@benesch
Copy link
Contributor Author

benesch commented Jun 5, 2017

Ok! Ready when you are, @sploiselle.

@sploiselle
Copy link
Contributor

Redcarpet's breaking on nested code blocks, e.g., Cluster Setup Troubleshooting.

Can we resolve this with changes to the markdown, or is it a Redcarpet issue?

@sploiselle
Copy link
Contributor

Didn't realize my last comment was already tracked in #1492; do you want to do a fast-follow PR to fix these issues @jseldess? Otherwise, I'm glad to take it

@jseldess
Copy link
Contributor

jseldess commented Jun 5, 2017

@benesch, Sean found another page where the indentation needs to be fixed up. Would you handle that real quick before merging? Otherwise, yeah, would be great if you'd follow up with a fix after merge, @sploiselle.

benesch added 5 commits June 5, 2017 11:18
Markdown is a very loosely-specified format, so there are understandibly
some differences in how parsers render the same Markdown file. Our new
Markdown parser, redcarpet, is picker than our old parser, Kramdown, and
insists on things like four spaces of indentation for paragraphs/code
blocks in lists.

This commit fixes several places where we were relying on Kramdown's
heuristics to instead adhere to the Markdown standard.
Kramdown and Redcarpet generate href hashes slightly differently;
adjust accordingly.
Now that we have four gem dependencies (jekyll, redcarpet, html-proofer,
and rake), it's annoying to install each by hand. Re-add a Gemfile that
lists our dependencies and versions. (The original Gemfile was removed
to fix build issues in TeamCity; I'll just fix those.)
@benesch
Copy link
Contributor Author

benesch commented Jun 5, 2017

Oh, good catch! Should be fixed now.

@cockroach-teamcity
Copy link
Member

@sploiselle
Copy link
Contributor

You are such a champ, @benesch

@benesch
Copy link
Contributor Author

benesch commented Jun 5, 2017

🍻

@benesch benesch merged commit bed4a05 into cockroachdb:gh-pages Jun 5, 2017
@benesch benesch deleted the fast-compiles branch June 5, 2017 15:40
benesch added a commit to benesch/docs that referenced this pull request Jun 30, 2017
I broke this in cockroachdb#1477. Guess it wasn't "dead code."
benesch added a commit to benesch/docs that referenced this pull request Jun 30, 2017
I broke this in cockroachdb#1477. Guess it wasn't "dead code."
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.

5 participants