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: TOC word-break not being inserted in generic types #8217

Merged
merged 1 commit into from
Oct 27, 2022
Merged

fix: TOC word-break not being inserted in generic types #8217

merged 1 commit into from
Oct 27, 2022

Conversation

juliansangillo
Copy link
Contributor

Expected behavior: Word-break opportunities are inserted into the API references in the TOC.
Actual behavior: Word-break opportunities are not inserted if the reference is a generic type, causing that link to overflow its container.

Before fix

before

After fix

after

These were taken from a template I am using. As shown in the before image, the second reference in the TOC on the left word-breaks correctly because it is non-generic, however the fourth and last references is generic and it overflows. I managed to fix it by overriding the docfx.js file in the template and applying the fix there. However, this is just a workaround and isn't an ideal one. It would be better if the fix was included in the default template so overriding this file becomes unnecessary. Also, for some more context, this issue is also present in DocFx's own API documentation site, not just the template I am using. As you can see below, there are quite a few generics in the TOC that overflow.

docfx-site

This issue is caused because the function that inserts the <wbr> (word-break opportunity) tags only does so if this.html() has no html tags and is only text. It does this by comparing this.html() with this.text() to see if they are equal. This only works if the text is a non-generic reference. If it is generic, then this.html() returns the reference with the less-than and greater-than symbols encoded as &lt; and &gt; respectively. this.text() doesn't encode those symbols, so the check fails and the tags are never inserted.

This fix replaces that check. Instead, it checks if this.html() doesn't match this regex: /(<\w*)((\s\/>)|(.*<\/\w*>))/g. This regex matches if there are any html tags. This regex has been tested with https://www.regextester.com/ and the following text:

<p>System.Collections.Generic.List&lt;string&gt;</p>
System.Collections.Generic.List&lt;string&gt;

The first line should match, the second one should not.

@paulushub
Copy link

It seems this (a modification of the docfx.js) should be handled in the custom templates.
Some will prefer an automatic horizontal scrollbar to the word-break.

@herohua herohua added the v2 label Oct 17, 2022
@juliansangillo
Copy link
Contributor Author

@paulushub The thing is, word-breaks are already an existing feature. This is just a bug fix for an issue where the word-breaks weren't being inserted for generic classes.

That is an interesting idea though. Although, not everyone may prefer that. I for one like the word breaks as I can see everything without having to scroll. Maybe including scrolling as opposed to word-breaks as an option in globalMetadata could be a feature request for other contributors to tackle?

I also disagree that this should be handled in custom templates. As I stated, this already exists in the default template, it just doesn't work for generics. Also, while I am not an owner of this project, it seems like bad practice to me to override the docfx.js file just to change this. That file has a lot more logic in it. What if any of it changes in future releases of docfx? Then the custom templates won't see the changes.

@yufeih yufeih merged commit 7a26026 into dotnet:dev Oct 27, 2022
@yufeih yufeih added the bug-fix Makes the pull request to appear in "Bug Fixes" section of the next release note label Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix Makes the pull request to appear in "Bug Fixes" section of the next release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants