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

Markdownlint: note-box-headings custom rule #28372

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

MaoShizhong
Copy link
Contributor

@MaoShizhong MaoShizhong commented Jul 9, 2024

Because

As discussed with staff, for better accessibility, note boxes should have headings which will make them click-linkable. These headings should have a consistent heading level enforced. Accessibility can also be improved by including the note box type as part of the heading (e.g. warning boxes with a heading like "Warning: Insert common pitfall here"), as we're currently only differentiating between types via border colour and small icons.

This PR

  • Adds TOP011 custom rule for note box headings
  • Provides test file demonstrating what headings are flagged and not flagged by this custom rule
  • Provides TOP011 doc file
  • Adds TOP011 to the custom rules array in the markdownlint config file
  • Enforces note box heading rules in the layout style guide

Issue

N/A

Additional Information

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project curriculum contributing guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

@MaoShizhong MaoShizhong added the Content: Markdownlint Involves anything related to the curriculum repo linter label Jul 9, 2024
@MaoShizhong MaoShizhong requested a review from thatblindgeye July 9, 2024 14:20
Original propose TOP011 closed
Now made generic for multiple aspects of note box headings
@MaoShizhong MaoShizhong marked this pull request as draft October 2, 2024 20:46
3 different types of note box heading errors now
- missing heading
- incorrect heading level
- incorrect heading start text (type-dependent)

Will help make note boxes more accessible (linkable via heading id
fragment, and note box type not limited to visual differences and div class).
@MaoShizhong MaoShizhong marked this pull request as ready for review October 2, 2024 21:57
@MaoShizhong MaoShizhong changed the title Markdownlint: note-box-heading-level custom rule Markdownlint: note-box-headings custom rule Oct 2, 2024
@MaoShizhong
Copy link
Contributor Author

Rule converted from heading level to a general note box heading rule. Only the heading level error is practical to fix via a script.

@MaoShizhong MaoShizhong removed the request for review from thatblindgeye October 3, 2024 16:21
@MaoShizhong
Copy link
Contributor Author

Point for consideration:

Prior staff discussion highlighted the value in accessibility for making note box headings mandatory and including the note box type in the heading. If we land this rule, existing note box headings will need to be converted when they flag the lint error. However, MD051 (the lint rule that checks for valid link fragments) was disabled in #28701. Therefore, any existing links to the old note box heading will be invalid if not also changed, and the lack of MD051 means the linter won't catch the broken link. This would typically occur with links in the Knowledge Check section if they reference a note box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content: Markdownlint Involves anything related to the curriculum repo linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant