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

fix(folders): Use real folder names instead of splicing slugs #1691

Open
wants to merge 1 commit into
base: v4
Choose a base branch
from

Conversation

necauqua
Copy link
Contributor

In breadcrumbs and folder pages (both folder page titles and page lists) the name of the folder was derived from the slug unless overriden, which is.. wonky.

This is much more noticeable if you change the slugify function to make all slugs lowercase - which I did, and which may be a followup PR.

The patch was pretty straightforward though, we just use the real folder names from the relativePath.

Just noticed that the explorer did not have this issue and it did use the relativePath just like I'm doing here, so this is also consistency ig

With this fix lowercase slugs seem to work?. Other than that I haven't noticed any issues yet.

@aarnphm
Copy link
Collaborator

aarnphm commented Dec 30, 2024

fwiw we should get from title from frontmatter for folder that contains index.md, otherwise iirc the reason why we slice slug is that slug is final/atomic (i might be wrong here, cc @jackyzha0 on this)

@jackyzha0
Copy link
Owner

yeah we use the title by default and fallback to slug segment, im ok changing it to real folder name

@necauqua
Copy link
Contributor Author

we should get from title from frontmatter for folder that contains index.md

In breadcrumbs we do that and my change does not break it.

The other thing is folderPage title and nested folders in the page list - currently those just use slugs, we should add resolving folder titles from index.md frontmatter there too.

slug is final/atomic

Slugs are for better URLs, and being final/atomic makes total sense, since we use slugs as keys for lookups etc, sure.

But this change is only for what's emitted in the final html visually (esp. the folderPage title where it's not even a link lol) - slugs there look very ugly - and even surprising(!), compared to actual directory names they come from.

As I've mentioned, this is especially noticeable if you want lowercase slugs for nicer URLs - which btw is very easy to do, just add a little .toLowerCase() to the slugify function, I havent noticed any issues besides visual folder names so far

@necauqua
Copy link
Contributor Author

Also I think resolveFrontmatterTitle option in Breadcrumbs is not needed, the doc says there's performance issues, but all it does is add a single loop through the content (a single per build, not per component instance aka per page, since it's cached), which a lot of plugins/components do
Maybe I'm missing something tho

@jackyzha0
Copy link
Owner

Also I think resolveFrontmatterTitle option in Breadcrumbs is not needed, the doc says there's performance issues, but all it does is add a single loop through the content (a single per build, not per component instance aka per page, since it's cached), which a lot of plugins/components do Maybe I'm missing something tho

we should update the docs, i fixed the perf there but forgot to update it lol

@necauqua
Copy link
Contributor Author

Well if the option was there for perf issue that was fixed we could just remove it

Maybe leave it in the interface for it to not be a breaking change for a while and just ignore it

@jackyzha0
Copy link
Owner

Well if the option was there for perf issue that was fixed we could just remove it

Maybe leave it in the interface for it to not be a breaking change for a while and just ignore it

turns out i updated the docs but not the docstring in the actual interface, feel free to just remove that warning, i think keeping the option there is fine 🤷

@necauqua
Copy link
Contributor Author

I think the option is useless, because you need to explicitly make an index.md and put a title in there to make use of the feature, so having an option to enable that is redundant.

It made some sense with the perf issue, but now it only technically saves you some unnoticeable amount of time 🤷

But if you're sure you want to keep it then okay

In breadcrumbs and folder pages (both folder page titles and page
lists) the name of the folder was derived from the slug unless
overriden, which is.. wonky.

This is much more noticeable if you change the slugify function to make
all slugs lowercase - which I did, and which may be a followup PR.

The patch was pretty straightforward though, we just use the real
folder names from the relativePath.
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