Feature Request: Level 4 headings render h4 > a with fragment (and id)

MaoShizhong commented 2 months ago

Checks

[X] I have thoroughly read and understand The Odin Project Contributing Guide
[X] The title of this issue follows the Feature Request: brief description of feature request format, e.g. Feature Request: Add a dark mode to the website
[X] Would you like to work on this issue?

Description of the Feature Request

Currently, Kramdown converts level 3 0-indent headings to h3 > a with a parameterized fragment href, and provides its parent section with the parameterized id for scrolling. Level 4 headings do not generate their own link and fragment, meaning several lessons have resorted to doing things like adding

<span id="subsection-title"><span>

below the level 4 heading for things like knowledge checks.

An issue to consider is that simply amending lib/kramdown/convert/odin_html.rb to

HEADER_LEVELS_TO_CONVERT = [3, 4].freeze

# ... inside #convert_header
if HEADER_LEVELS_TO_CONVERT.include?(element.options[:level])
  # ...

  format_as_block_html("h#{element.options[:level]}", element.attr, body, indent)

is insufficient, as there isn't a good place to put the matching id (since the h3's generated id gets placed on the parent section). Note that the tailwind config and lesson_content.css would need some minor alterations to include h4 in the relevant rules.

A possible non-intrusive workaround

Do the above and move the generated id to the h(3,4) > a:

def convert_header(element, indent)
 if HEADER_LEVELS_TO_CONVERT.include?(element.options[:level])
   section_anchor = generate_id(element.options[:raw_text]).parameterize
   body = "<a#{html_attributes({ href: "##{section_anchor}", id: section_anchor, class: 'anchor-link' })}>#{inner(element, indent)}</a>"

   format_as_block_html("h#{element.options[:level]}", element.attr, body, indent)
 else
   super
 end
end

Replace section id with data-title:

def content
 "<section data-title='#{title.parameterize}' markdown='1'>#{formatted_content}</section>"
end

Which produces the following DOM structure

Amend lesson_toc_controller.js to account for change of section attribute:

// LessonTocController.prototype.connect
this.lessonContentTarget.querySelectorAll('section[data-title]')

// callback in LessonTocController.prototype.activeSection
const { title } = entry.target.dataset;
const tocItem = this.tocTarget.querySelector(`li a[href="#${title}"]`).parentElement;

If ids are removed from sections or changed to data-title, I'm not 100% sure if there are any further impacts on any other part of the app (e.g. document_sections.rb:32 passing in title: 'content' for unsectionable_content). A cursory and inexperienced look seems like it wouldn't, but more experience likely would know.

Acceptance criteria

[ ] Have Kramdown:Convert:OdinHtml#convert_header act upon both heading levels 3 and 4
[ ] Include generated id in the h(3|4) > a
[ ] Replace parent section id with data-*. Amend class and .new calls as necessary.
[ ] Amend lesson_toc_controller.js to account for sections having data-* instead of id.
[ ] Adjust tailwind.config.js and lesson-content.css to account for both h3 and h4 instead of just h3

Additional Comments

An issue I can imagine might crop up is if any lessons have bare spans or bare anchors (some have <a id="fragment"></a> or <a name="fragment"></a> instead of spans) that already have the id that would be generated by a level 4 heading. querySelector will still select the first id match which likely won't be an issue, but it would still be worth then grepping the curriculum for bare anchors/spans with ids/names and see if most/all can be removed if this feature is implemented.

MaoShizhong commented 2 months ago

EDIT: Resolved by further amending lesson-content.css

  /* old */
  .lesson-content section[id] {
    scroll-margin-top: 40px;
  }

  /* new */
  .lesson-content a[id] {
    scroll-margin-top: 40px;
  }

Just came to mind that putting the id in the anchor instead of the parent section will cause scrolling to scroll to the very top of the heading, without any top margin (since the h3 will have a top margin between it and the parent section) and that seems to mess with the intersection observer for the side list, where being on a section has 2 sections highlighted.

Still familiarising myself with how the JS controllers actually work to see if there's a simple solution there.

KevinMulhern commented 2 months ago

Thanks for the detailed issue @MaoShizhong. I might be misunderstanding the problem (very likely lol), But we should be generating ids for h4's, I think Kramdown does it by default and we only override it for h3's - This h4 can be used as an anchor link and linked to for example: https://www.theodinproject.com/lessons/node-path-intermediate-html-and-css-css-units#viewport-units.

Is the problem you'd like to solve with this making h4's clickable and behave like h3's in that way?

MaoShizhong commented 2 months ago

@KevinMulhern Yes, the primary goal is to make h4s clickable just like h3s, and it felt a little odd just having the h3s link to an id on the parent section, with h4s having their id on themselves. As opposed to both h3s and h4s' having the id on themselves, and using a data-* for the parent section.

I actually didn't clock that the h4s generate their own ids because when I was looking at them, I was looking via the markdown preview tool, in which the current CSS Units lesson markdown produces this:

But looking at it now, it also doesn't generate sections like with the real page. I think I actually missed the lack of sections in the markdown preview tool because I then started looking at how the CSS and Kramdown changes proposed above affected the real page, and so didn't look at the markdown preview tool anymore :sweat_smile:

MaoShizhong commented 2 months ago

If it won't have any impact elsewhere, then perhaps to make h4s clickable like h3s, it might make sense then to not override Kramdown's h3 id generation?

Then we could have both h3s and h4s render a child anchor, have the h3s and h4s have their ids generated by default, then just change the parent sections' ids to data-title or equivalent. Then the relevant CSS can be amended to account for these.

The end result should hopefully then be no change to site behaviour (no change to the "current section" list), but then both h3s and h4s are clickable.

KevinMulhern commented 2 months ago

Makes a lot of sense to me @MaoShizhong. Changing the sections to use a data attribute would be a nice refactor and I agree it would make it clearer. The toc is the main thing that will be impacted, but it sounds like you have that well covered. Lets do it!

MaoShizhong commented 2 months ago

@KevinMulhern any ideas about the same lesson file producing different HTML.via the markdown preview tool vs when done via the curriculum:update_content process?

KevinMulhern commented 2 months ago

Its got something to do with the html santisation we added to the preview tool recently I bet. I'll have a look into it 👀

MaoShizhong commented 2 months ago

Want me to open an issue for that for record-keeping?

KevinMulhern commented 2 months ago

Thanks @MaoShizhong, I've got a fix here. It wasn't as bad as I was expecting lol

TheOdinProject / theodinproject