Skip to content

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Jan 7, 2025

No description provided.

@Mpdreamz Mpdreamz linked an issue Jan 7, 2025 that may be closed by this pull request
@Mpdreamz Mpdreamz mentioned this pull request Jan 7, 2025
@Mpdreamz Mpdreamz requested a review from reakaleek January 7, 2025 19:00
@bmorelli25
Copy link
Member

bmorelli25 commented Jan 7, 2025

Looks good! Definitely good enough for MVP. A few thoughts, likely for a future enhancement issue:

  • We should figure out the approach we want to take with deeply nested/long heading pages. I think a scroll is fine for now, but we likely want to do something different in the long run.
Screenshot 2025-01-07 at 12 13 19 PM
  • We probably want to use navigation_title for both the TOC and the Breadcrumbs. This will greatly reduce the length of some of our longer breadcrumb sequences and allow us to prioritize SEO without worrying about the length of page titles. It also creates a visual mapping between breadcrumb/TOC heading.

  • A lot of breadcrumbs omit the current page you're on. This reduces duplication and further shortens long breadcrumb sequences. For example, instead of this:
    Screenshot 2025-01-07 at 12 20 30 PM

    We can do this:
    Screenshot 2025-01-07 at 12 20 30 PM

Again, I don't think any of this needs to be addressed in this PR. Weigh the amount of effort required vs opening an issue and adding these enhancements later. I think what you have here is great for MVP.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Jan 7, 2025

Those are great points and except for the first one quick wins, will amend in the morning!

@reakaleek
Copy link
Member

Do you think a test for this could be beneficial?

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Jan 8, 2025

Do you think a test for this could be beneficial?

yes! good call calling out my laziness 😸


doc!.Parent.Should().NotBeNull();

doc.YieldParents().Should().HaveCount(2);
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think another critical path worth testing is if the page has 0 parents. Nevertheless, thank you for adding a test!

Copy link
Member Author

Choose a reason for hiding this comment

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

++ will follow up at some point!

@Mpdreamz Mpdreamz merged commit d4f6654 into main Jan 8, 2025
3 checks passed
@Mpdreamz Mpdreamz deleted the feature/bread-crumbs branch January 8, 2025 16:18
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.

Breadcrumbs don't work
3 participants