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: Bad code fence indenting in ordered lists #675

Merged
merged 1 commit into from
Nov 24, 2020

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Oct 20, 2020

Why:

Split out commit from #541 for markdown sections that cause parser issues with markdownlint because of incorrect indenting of start/end code fences

What's being changed:

  • Un-started table row. Caused Crowdin to mangle it in the translated copies
  • Code fences inside of ordered list that didn't have at lease 3 spaces and some that had different open/closing indents

Check off the following:

@janiceilene
Copy link
Contributor

janiceilene commented Oct 20, 2020

Thanks for opening a PR @nschonni! I'll get this triaged for review 🌟

Edit: I'll wait to triage this one until we get a chance to look at the linked PR 💛

@nschonni
Copy link
Contributor Author

@janiceilene I've removed the content changes from the other PR now, since the original issue requiring them has been fixed in Markdownlint. They are still content issues, so I'll leave the for review here

@janiceilene janiceilene added the hacktoberfest-accepted We might not merge this PR before Nov 1st, but it's a wonderful Hacktoberfest contribution! label Oct 27, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2020

This PR is stale because it has been open 7 days with no activity and will be automatically closed in 3 days. To keep this PR open, update the PR by adding a comment or pushing a commit.

@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Nov 4, 2020
@nschonni nschonni force-pushed the code-fence-indents branch 2 times, most recently from 4351cf6 to 1fdcec6 Compare November 4, 2020 18:39
@@ -27,7 +27,7 @@ Before launching {% data variables.product.product_location %} on Google Cloud P
{% data variables.product.prodname_ghe_server %} is supported on the following Google Compute Engine (GCE) machine types. For more information, see [the Google Cloud Platform machine types article](https://cloud.google.com/compute/docs/machine-types).

| High-memory |
------------- |
| ------------- |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only functional change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this got Crowdin to parse correctly again from the changes in #1603

@nschonni
Copy link
Contributor Author

nschonni commented Nov 4, 2020

@janiceilene @rachmari looks like the stale bot doesn't want to dismiss the label after pushing/rebasing

@rachmari rachmari removed the stale There is no recent activity on this issue or pull request label Nov 4, 2020
@nschonni nschonni force-pushed the code-fence-indents branch 3 times, most recently from 71c6c87 to bf19b22 Compare November 6, 2020 22:56
@janiceilene
Copy link
Contributor

Looking at this again briefly, this can be reviewed by any writer! I removed @rachmari and I as reviewers and I'll move this to the anyone column for triage.

@janiceilene janiceilene added the content This issue or pull request belongs to the Docs Content team label Nov 9, 2020
Copy link
Contributor

@martin389 martin389 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @nschonni! I've added a couple of suggestions as comments.

@nschonni nschonni force-pushed the code-fence-indents branch 3 times, most recently from 3f279e6 to 33c2fcc Compare November 19, 2020 00:16
@janiceilene
Copy link
Contributor

👋 @martin389 It looks like this one's ready for your 👀 again

@nschonni nschonni requested a review from martin389 November 19, 2020 23:43
@martin389
Copy link
Contributor

martin389 commented Nov 20, 2020

Hi @nschonni 👋

I had a look at Configuring high availability replication for a cluster, and some of the rendering might need to be checked:

  • In step 12 of Adding passive nodes to the cluster configuration file, the contents of the warning admonition seem to have escaped and are rendering underneath the block:
    image
    This has also reset the procedure's numbering back to 1, when it should be continuing to step 13 from there.

Copy link
Contributor

@martin389 martin389 left a comment

Choose a reason for hiding this comment

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

Thanks for these changes, @nschonni! 🎉

I've finished my review, and added a comment above for one of the sections:

The rest looks good to me: 🚀

@nschonni
Copy link
Contributor Author

🤦 sorry about the repeat reviews. I rebased and checked that file on dev side-by-side again with poduction and they're looking the same.
I notice that there appears to be something funny going on with the list numbering with the warning/note blocks resetting the numbering, but it appears in production as well. It looks like the correct <ol start='#'> is being correctly set, but the styling is going back to 1 🤷

@martin389
Copy link
Contributor

martin389 commented Nov 23, 2020

@nschonni

🤦 sorry about the repeat reviews. I rebased and checked that file on dev side-by-side again with poduction and they're looking the same.
I notice that there appears to be something funny going on with the list numbering with the warning/note blocks resetting the numbering, but it appears in production as well. It looks like the correct <ol start='#'> is being correctly set, but the styling is going back to 1 🤷

No worries, thanks so much for checking on this 🚀
It does seem like it should all be rendering as expected, so I'll do some digging around here too.

@martin389
Copy link
Contributor

👋 For docs tooling folks:

The procedure numbering resets a few times in this staging article: "Configuring high availability replication for a cluster". Specific examples are listed below:

  • "Assigning active nodes to the primary datacenter": (stage) (prod) - Step 5 in prod becomes step 1 in stage."
  • "Adding passive nodes to the cluster configuration file": (stage) (prod) - Step 2 in prod becomes step 1 in stage. Numbering also resets again a few more times throughout this section.

@nschonni
Copy link
Contributor Author

Sounds like it might have been an internal issue that was fixed #1552 (comment)
I'll try rebasing now to see if it fixes it

@nschonni
Copy link
Contributor Author

@martin389 rebase looks like it fixed it 😄

Copy link
Contributor

@martin389 martin389 left a comment

Choose a reason for hiding this comment

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

@martin389 martin389 merged commit 93c24ac into github:main Nov 24, 2020
@github-actions
Copy link
Contributor

Thanks very much for contributing! Your pull request has been merged 🎉 You should see your changes appear on the site in approximately 24 hours.

@martin389
Copy link
Contributor

Thanks again for these fixes, @nschonni! 🎉

@nschonni nschonni deleted the code-fence-indents branch November 24, 2020 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content This issue or pull request belongs to the Docs Content team hacktoberfest-accepted We might not merge this PR before Nov 1st, but it's a wonderful Hacktoberfest contribution!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants