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

1068 Tabs Have Got To Go #1268

Merged
merged 27 commits into from
Jan 2, 2025
Merged

1068 Tabs Have Got To Go #1268

merged 27 commits into from
Jan 2, 2025

Conversation

SallyMcGrath
Copy link
Member

@SallyMcGrath SallyMcGrath commented Dec 28, 2024

What does this change?

Removes old tabs and replaces with a simpler format that is less prone to breaking its inner shortcodes.

Why does it work better? Ohhhh, well, it just reduces the number of renders it has to go through which could screw up and escape html into the view, basically. And it's a bit easier to type as pressing the shift key is hurting my hands these days.

Why reduce renders? What does this mean? How does Hugo render content? Like most template engines, it more or less renders from the innermost piece and works its way outwards. Hooks, shortcodes, partials, layouts, base. At any point you might | markdownify a string. If you do nothing, all the strings will accumulate into .Content on the Page and be rendered there. Hugo will try to guess what is markdownifiable at this point. Typically then it gets some things wrong and we humans fix this by marking strings to be explicitly markdownified before they blobulate into .Content.

https://gohugo.io/functions/transform/markdownify/

But where this goes wrong is twofold:

  1. Markdownify only processes the markdown in front of it. It can't process the actual Page context, so if your markdown accesses the page context, for example to make a link or call an image, it could go wrong.
  2. HTML escaping -- markdownify cannot cope with whitespace, because markdown has, stupidly, whitespace formatting tricks (which should be illegal). So any time you are markdownifying you had better also rigorously chomp your whitespace in any HTML templating. In Hugo you do this with a dash{{-

You can stop here and be happy with your life. Or you can read on and fill in your Hugo model a bit and understand why this ranges all over so many files when it claims to just be about tabs.

67 files she changed, 47 markdown

So why are we using markdownify if it's so rubbish? Well, maybe we shouldn't any more. It's just… how it used to work. It used to be the only way to do this. It's also explicit -- it's easier to see that we want whatever is in this variable or dot to probably format as text or code or links etc.

https://gohugo.io/functions/transform/

So what's the alternative to markdownify for partials that contain markdown?

https://gohugo.io/methods/page/renderstring/

RenderString is now what markdownify does under the hood nowadays. It used to work quite differently to markdownify. (You can also pass different rendering options like pandoc but let's not get distracted.) Whenever you get problems with your markdownify, someone will suggest using $.Page.RenderString instead. It's used like z-index:1000 in CSS - like magic beans. But this isn't helpful often because

It requires a Page object ! (And that's where this falls down)

It's clearly a method on Page. It simply doesn't apply to rendering outside this context. It will fail out on most of the places we transform md as those aren't Pages. (Like, all remote blocks). But then how does markdownify work if I'm saying it's RenderString secretly anyway? Well, it sets the page context to the site home page and pretends.

Sooo we can use page. For many contexts we can and should use page.RenderString. We should use it whenever we are operating on normal partials rendering local markdown content. Even with the block system, since 0.111.0 we can use lowercase page to access "the current page" even if that's out of scope!

So I've done that

https://github.com/CodeYourFuture/curriculum/pull/1268/files?file-filters%5B%5D=.html&show-deleted-files=true&show-viewed-files=true

https://gohugo.io/functions/global/page/

But notice, scroll down, we shouldn't use this with shortcodes or we might end up with cached rendered shortcodes displaying on other pages with the wrong insides. And shortcodes are where all our pain resides.

Yonder magic beans are ye

While we rest here, quiet and sad, another magic bean is the {{% render first syntax %}}. Typically we write shortcodes with angle brackets {{<shortcode>}} . If we use the % then the shortcode is rendered and passed to goldmark as html. Another way to say this is < means "contains only html" and % means "contains markdown and html". Even though in practice it's fine to pass html into < because you are dealing with that in your shortcode. This is almost never relevant to your problem but will be suggested frequently.

SafeHTML, she cried

Another magic bean, applied randomly, is safeHTML. Does nowt in almost all of these cases because markdownify, RenderString and .Content already return safeHTML. It's really meant for when you are constructing a string with, say, printf and then you want that parsed as html subsequently.

https://gohugo.io/functions/safe/html/

Whitespace crisp and even

Weirdly, the most unloved magic bean – chomping your whitespace – is nearly always the right answer for us. {{-. This is because goldmark will interpret whitespace as either a paragraph, blockquote, or code block. It will then close out your parent shortcode and dump your child shortcode inside that phrasing block, which will either look broken or actually be broken. This is the main risk of transforming at the component level and the main solution is to clean up your whitespace. This could include moving comments to the bottom of a file. It could also include passing RenderString the opt "inline".

So, you might wonder at this point why we are transforming per partial instead of bringing everything up to a page and then rendering on there. I wondered this too, and I tried it, and everything broke. I felt so weary at this point that I could go no further.

Tread thou in them boldly

You can explore how this works by removing the markdownify transform from note.html. On a day plan, say, the notes coming from a readme block will be transformed, but the local notes will not. This is because the readme notes are being transformed anyway by the markdownification in readme.html. There's no markdownify in local.html, so the note comes through untransformed. So, should local be markdownified? If we write a note shortcode in a page, just any content page, it will be rendered via .Content. And in fact all the shortcodes are easily and beautifully rendered inside each other at this level inside Page. It all Just Works. But we do want to compose pages from multiple sources including remote sources, so here we are.

I updated the markdownify calls outside of shortcodes to either .Page or page .RenderStrings and went forth through the rude winds' wild lament.

And the bitter weather.

Render hooks and bring me wine

But this is Hugo, so what is the confounding exception? Oh, that's render hooks! Render hooks hook IN to the render and extract pieces from it, to be processed separately. That's how syntax highlighting works in code fences, but you can hijack any segment of markdown and process it first, then pass it up through all your contexts as safeHTML. Render hooks are almost a better version of shortcodes. It feels like they are. I prefer them in most cases as they don't break so often or as weirdly. But of course they do break. If you pass a shortcode into a render hook by using, for example, a GFM note and putting a youtube video inside it, then your hook might simply give up, close itself out, and dump the nested shortcodes underneath. So really render hooks should be terminal components only? GFM Notes don't really fit in this paradigm, but they exist, so…. Hahahahugo!

Merry Christmas!

Common Content?

Common Theme?

Fixes #1068
Addresses #847

Org Content?

Module | Sprint | Page Type | Block Type

Checklist

Who needs to know about this?

so as to not lose it
might also go to %%
now tabs are renderered differently we could trigger accidental escapes via whitespace -- have  done this on the quiz but will look for more
we will never use tabs in an external context anyway as they need js
reduce chance of hugo wrapping these inline elements with p tags
moving comments seems to be the last thing?
because they are threatening to deprecate it
Copy link

netlify bot commented Dec 28, 2024

Deploy Preview for cyf-programming ready!

Name Link
🔨 Latest commit 3e3bcc2
🔍 Latest deploy log https://app.netlify.com/sites/cyf-programming/deploys/6776614196dc7b0008ffd447
😎 Deploy Preview https://deploy-preview-1268--cyf-programming.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 98 (🔴 down 1 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 90 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 28, 2024

Deploy Preview for cyf-sdc ready!

Name Link
🔨 Latest commit 3e3bcc2
🔍 Latest deploy log https://app.netlify.com/sites/cyf-sdc/deploys/67766141b064e50008fca120
😎 Deploy Preview https://deploy-preview-1268--cyf-sdc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 28, 2024

Deploy Preview for cyf-piscine ready!

Name Link
🔨 Latest commit 3e3bcc2
🔍 Latest deploy log https://app.netlify.com/sites/cyf-piscine/deploys/677661411ca1b000086a5498
😎 Deploy Preview https://deploy-preview-1268--cyf-piscine.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 28, 2024

Deploy Preview for cyf-common ready!

Name Link
🔨 Latest commit 3e3bcc2
🔍 Latest deploy log https://app.netlify.com/sites/cyf-common/deploys/6776614179f1ff00087716e1
😎 Deploy Preview https://deploy-preview-1268--cyf-common.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 28, 2024

Deploy Preview for cyf-curriculum canceled.

Name Link
🔨 Latest commit 3e3bcc2
🔍 Latest deploy log https://app.netlify.com/sites/cyf-curriculum/deploys/67766141e72e3e00080f0a57

Copy link

netlify bot commented Dec 28, 2024

Deploy Preview for cyf-itd ready!

Name Link
🔨 Latest commit 3e3bcc2
🔍 Latest deploy log https://app.netlify.com/sites/cyf-itd/deploys/67766141eb8afb00085385c9
😎 Deploy Preview https://deploy-preview-1268--cyf-itd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 98 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 28, 2024

Deploy Preview for cyf-launch ready!

Name Link
🔨 Latest commit 3e3bcc2
🔍 Latest deploy log https://app.netlify.com/sites/cyf-launch/deploys/67766141f6e2360008d7e908
😎 Deploy Preview https://deploy-preview-1268--cyf-launch.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 28, 2024

Deploy Preview for cyf-tracks ready!

Name Link
🔨 Latest commit 3e3bcc2
🔍 Latest deploy log https://app.netlify.com/sites/cyf-tracks/deploys/677661410d5b8a0008b91371
😎 Deploy Preview https://deploy-preview-1268--cyf-tracks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@SallyMcGrath SallyMcGrath added this to the Common Theme Release milestone Dec 29, 2024
@SallyMcGrath
Copy link
Member Author

SallyMcGrath commented Dec 29, 2024

BTW this is "breaking" the deploys because this PR is being interpreted as unclosed shortcodes in the docs, sigh! The docs use the curriculum prs as content for the pr block example. Let's just merge it and it will heal itself.

Copy link
Member

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Looks great in theory! Seeing a few issues in practice...

common-content/en/module/js1/comparison/index.md Outdated Show resolved Hide resolved
common-theme/layouts/shortcodes/tabs.html Outdated Show resolved Hide resolved
@SallyMcGrath SallyMcGrath marked this pull request as draft December 30, 2024 16:56
@SallyMcGrath SallyMcGrath marked this pull request as ready for review January 1, 2025 13:48
@illicitonion
Copy link
Member

LGTM, ship it!!

One thing that is still broken (but were broken before) that I thought this was going to fix: #1052

https://deploy-preview-1268--cyf-programming.netlify.app/structuring-data/sprints/3/prep/#starting-a-project

Screenshot 2025-01-02 at 09 55 57

@SallyMcGrath
Copy link
Member Author

LGTM, ship it!!

One thing that is still broken (but were broken before) that I thought this was going to fix: #1052

That's definitely related so not unreasonable to think it- it's this central Hugo question of at which layer of the onion skin do we want this code to be rendered. In the case of ourname, we always want it to process first and pass to any other parent as safeHTML. We never want it to be markdown. Inside tabs it's processing the code first and then this shortcode. (If we move ourname out of tabs it renders fine. ).

Screenshot 2025-01-02 at 10 26 08

@SallyMcGrath SallyMcGrath merged commit f73a104 into main Jan 2, 2025
34 checks passed
@SallyMcGrath SallyMcGrath deleted the feature/1068-new-tabs branch January 2, 2025 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Tabs Have Got To Go
2 participants