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

Rewrite formatting help section db seed in Markdown #988

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

trichoplax
Copy link
Contributor

@trichoplax trichoplax commented Jan 25, 2023

Fixes several places where the rendered output is incorrect in the Formatting section of the help, and converts the whole database seed file to Markdown to reduce the risk of future errors and make the help section editable by non-technical site admins.

Running UPDATE_POSTS=true rails db:seed in my local dev environment successfully pulled through the Markdown into the Formatting section of the help, and the output was correctly rendered.

Prompted by the subtle bug highlighted by #986 - surplus blank lines are appearing due to the HTML and Markdown renderers having a difference of opinion. I'll omit the details of why, unless requested, because they become irrelevant once the file is Markdown only.

Since converting all the HTML to Markdown makes the diff useless, I've described below those changes that go beyond simply representing the same thing in Markdown.


Links section

  • I've manually text wrapped the code block so that it is readable on mobile without side scrolling. Unlike paragraphs, code blocks do not wrap automatically. Previously it required side scrolling even on desktop. Since this is user editable content I don't see an option for making it wider on desktop only, so I've made it narrow enough to be readable on either type of device.
  • Old line 15 ended in an apparently unintentional backtick, and the whole line was omitted from rendering. Removing the backtick has fixed this.
  • Old lines 18-19 used Markdown syntax inside HTML, even though they are intended to show as rendered, so the help page showed them as raw Markdown. They now show as rendered output as intended.

Headings section

  • Old lines 57 and 59 were preceded by blank lines, which meant the Markdown renderer converted them even though they are inside a code block and not meant to be rendered. They now show as raw Markdown as intended.

Changes that do not affect appearance

  • I've removed the id attributes from the headings. Markdown will create these automatically, but with slightly different names. (This is not the case - I was confusing general Markdown with how Markdown is rendered on GitHub.) This will only be a problem if they have been linked to from elsewhere (there are no internal links to break).
  • I've removed the class attributes. These had values such as "hljs-string" and "hljs-symbol". They did not appear to be having any effect on the rendered output, and appear to have been added by an automated process that did not realise it was dealing with prose rather than code. Removing them allowed finishing the conversion to Markdown.

Changes that do affect appearance, but that I anticipate being acceptable

  • I've replaced all of the <h3><strong> headings with ## headings, which render as <h2>. This avoids the semantic problem of skipping <h2> (the post title will be <h1>), and avoids the use of the semantic <strong> tag in a context it was not intended for. This should make the page easier to parse for screen readers. The visual result is slightly larger section headings.

@trichoplax
Copy link
Contributor Author

I can't find a reason not to make this change, but to double check I've posted on Collab: Do seed posts need to be HTML rather than Markdown?

@cellio
Copy link
Member

cellio commented Jan 25, 2023

These sound like good changes. I haven't pulled it yet to take a closer look; just a couple things based on your summary.

I've asked in Discord about the reasons for help being HTML, just in case there's some barrier to converting at all. (Edited to add: thanks for asking this on Collab.)

I'd rather keep the id attributes. We don't know that there aren't links to them, either in other customized help topics or in posts on our communities. Is there a way to avoid breaking any links that might exist and still switch to Markdown? (Worst case is we use <h2> for these instead of ##, right? Not ideal but not fatal.)

Proper heading semantics (1, 2, 3, no skips) sounds like a win to me. I don't care if the text is a little bigger; it might make the page easier to scan.

@trichoplax
Copy link
Contributor Author

I'd rather keep the id attributes. We don't know that there aren't links to them, either in other customized help topics or in posts on our communities. Is there a way to avoid breaking any links that might exist and still switch to Markdown? (Worst case is we use <h2> for these instead of ##, right? Not ideal but not fatal.)

I was wrong. I thought that ids would be automatically inserted by the Markdown renderer, but it seems not. On GitHub, Markdown files have automatic ids for every heading level, but apparently that's a GitHub thing rather than a Markdown thing.

Options I can see:

  1. Go back to using <h2> as you suggest, with the old ids
  2. Check all the other documents (including all posts in all communities...?) to see if any are using internal links or links to sections of other documents. If none are, we can use ##. If some are, we can insert ids just for those cases
  3. Keep all as ## without checking. Any existing links will half break. They will still take you to the correct page, just not scroll to the relevant section (more of a problem the longer the target page is)

I'm not fond of the first 2 options (for the inconsistency), but they would work. I'd possibly be comfortable with option 3 if we could be reasonably sure there were very few links or that they were only to short documents (maybe if we could run a regex over the database to find links with a # in and manually check through the results).

Are you solely concerned about existing links, or would you still want the potential even if no links had been made yet?

@cellio
Copy link
Member

cellio commented Jan 25, 2023

I'm concerned about existing links, not so much future ones, but you're right that the page would still load so they're only half-broken.

(It would be nice to have in-page anchors, something that's been requested for headings within regular posts too, but that's a larger issue. If that happens then I hope whatever the solution is would work for help too, but we don't need to block on "somebody might want to direct-link this section".)

@cellio cellio requested a review from a team January 27, 2023 01:50
@ArtOfCode-
Copy link
Member

AFAIK, seeds still need to be written in HTML so that they seed and render correctly. @trichoplax sounds like you've found different here, can you confirm?

@trichoplax
Copy link
Contributor Author

trichoplax commented Jan 30, 2023

@ArtOfCode- At the time of writing I had tested this in development for the Formatting help page only. I've now tested for a few others, and discovered a subtlety I had missed previously (in bold italics below). The following have been true for each help page I have tested so far:

  • Editing the seed file to contain Markdown instead of HTML and then running UPDATE_POSTS=true rails db:seed results in the help page rendering correctly.
  • Editing that help page through the user interface shows that it is now stored internally as Markdown, and can be edited as such.
  • Saving that edit results in the help page rendering correctly.
  • Once a help page has been edited through the user interface, it is skipped on subsequent runs of UPDATE_POSTS=true rails db:seed if the edited version (not the seed) contains text outside of HTML tags (even plain text with no other Markdown).
    • Adding Markdown in the seed file does not cause it to be skipped on the initial run of UPDATE_POSTS=true rails db:seed. It is also not skipped on the next run, even though the site version of the help file now contains Markdown. It is only skipped after an edit specifically through the user interface that results in the file containing Markdown (even if that's an empty edit to a file that already contains Markdown from the seed file - it won't be skipped until the empty edit is performed).
    • Adding an HTML tag through the user interface does not result in the help page being skipped on the next run of UPDATE_POSTS=true rails db:seed.
    • Once skipping of a help page has been triggered once, editing it through the user interface to no longer contain anything outside of HTML tags does not stop it continuing to be skipped. After being skipped once, it continues to be skipped every time regardless of what you change through the user interface.

Note that the filename still ends .html - it is only the content which I've been changing to Markdown.

I don't know why the presence of non-HTML content blocks the seeds from updating the help pages, but only after a user edit. If there isn't a good reason for this behaviour, should it be considered a bug? If fixed it looks like it would allow all the help pages to be in Markdown and easily editable.

@ArtOfCode-
Copy link
Member

@trichoplax IIRC that's intended behaviour - if a help file has been edited by a community, we deliberately don't apply any seeded updates to it to avoid overwriting community-specific changes. Sounds like that's working as intended. Also sounds like replacing everything with Markdown does actually work.

@trichoplax
Copy link
Contributor Author

trichoplax commented Jan 30, 2023

(I posted this one before seeing your reply)


In the meantime, does this need to block this pull request?

  • The problem (user edits containing Markdown block future seeding) already applies to the existing HTML-only seed files.
  • I'm expecting that the seed files be used when setting up a new instance, rather than being used to overwrite existing user edited help files.

@trichoplax
Copy link
Contributor Author

@trichoplax IIRC that's intended behaviour - if a help file has been edited by a community, we deliberately don't apply any seeded updates to it to avoid overwriting community-specific changes. Sounds like that's working as intended.

Ah I see. Then in that case the bug is that the prevention of overwriting community-specific changes is broken when the community-specific changes are solely HTML.

@ArtOfCode-
Copy link
Member

Agree. No need to block this PR, then - has conflicts but once those are resolved this can be merged.

@cellio
Copy link
Member

cellio commented Jan 30, 2023

I got a little lost in the thread, but it sounds like this change will not cause overwrites of locally-modified help, right? I know at least one of our communities has heavily revised the help and others have made smaller changes, and we don't know what other deployments might be doing.

@trichoplax
Copy link
Contributor Author

@ArtOfCode- I've fixed the merge conflict but please hold off merging pending @cellio's concern.

If a community has made edits through the user interface, and if they have kept strictly to only using HTML, then it seems that their changes would be overwritten by UPDATE_POSTS=true rails db:seed.

I guess we can either:

  • back up their customised wording before deploying
  • edit their customised wording to contain at least one Markdown paragraph to prevent overwriting
  • hold off merging this change until the bug that allows overwriting customised wording is fixed

I'd lean towards holding off rather than have to think about other instances we might not know about (and therefore can't warn) getting unexpected data loss.

@trichoplax trichoplax marked this pull request as draft January 30, 2023 20:15
@cellio
Copy link
Member

cellio commented Jan 30, 2023

I'd lean toward holding off, too. Is there an issue for the other bug? If not, could you create one @trichoplax?

@trichoplax
Copy link
Contributor Author

I've raised #992 and changed this pull request to a draft until the bug is fixed.

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.

3 participants