Tooltip added for sidemenu subitems

[kumaravel95]

Summary
On hovering side menu items, It should tooltip the menu item. because long menu item text not fully visible in the screen #1178

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Code style update
• Refactor
• Docs
• Build-related changes
• Other, please describe:

Before fix:

After Fix:

Does this PR introduce a breaking change? (check one)

• Yes
• No

The PR fulfills these requirements:

• On hovering side menu items, It should tooltip the menu item. because long menu item text not fully visible in the screen On hovering side menu items, It should tooltip the menu item. because long menu item text not fully visible in the screen #1178

You have tested in the following browsers: (Providing a detailed version will be better.)

• Chrome
• Firefox
• Safari
• Edge
• IE

[kumaravel95]

gitpod url: https://gitpod.io/#https://github.com/kumaravel95/docsify

[anikethsaha]

Thanks a lot for the enhancement. Keep bringing them 😃

We have already crossed our size limit and we are not favoring any enhancement as of now.

can you create this as your own plugin and add a link to awesome-docsify !

[trusktr]

This is super useful!

I think we should update the style, as the black tooltip is fairly jarring on top of the white background.

Generally speaking we would need this change to integrate with all existing themes. Each theme CSS file should have styling for it.

cc @jhildenbiddle on styling

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/5yidjnetd
✅ Preview: https://docsify-preview-git-fork-kumaravel95-develop.docsify-core.vercel.app

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit fe898bb:

Sandbox	Source
serene-pascal-ishy7	Configuration

[trusktr]

@anikethsaha I don't think the size limit should affect what features we want to ship.

This could somehow be made a plugin, but that would greatly complicate the implementation of the feature. How would you write this as a plugin?

@jhildenbiddle mentioned in some other topic and I agree: we can adjust the stated size of the lib. How important is it to keep the limit to 26kb?

This is a really really nice feature, and we may discourage contributors and innovation if no new features can be added. Plus if size matters, then the server-side rendering should take care of a big size chunk (markdown marked renderer is needed only in the server, not the client, etc).

I just looked at the implementation, it is nice and simple as pure HTML/CSS. 👍 A plugin would require JS too.

I notice custom styling for vue.style, which is in the direction we should go for each style. Let's make it look good on all the themes before we merge this.

[anikethsaha]

Actually a while back, I was literally focussing on the size limit. Did miss some features while concerning that.
Ok cool let's review this, it's hardly 4-5 lines of JS.

@kumaravel95 , I could see some delay while tooltip being hide/visible. Can you minimize that?

Also how about configuring the position of the tooltip or changing it. Cause currently, it is hiding the content above the link, it would have been fixed if the tooltip is being rendered in the side.
Could you try that ? (non-blocker request. )

[trusktr]

it is hiding the content above the link, it would have been fixed if the tooltip is being rendered in the side.
Could you try that ? (non-blocker request. )

I would consider it a blocker. Let's not merge until we are satisfied with the result (and we can pitch in too). It should work well (positioning fixes, not blocking content) and look good on each theme before we merge.

@kumaravel95 Thanks for your initial work on this! Now let's just polish it up so it looks clean and works well.

[jhildenbiddle]

All new features and significant behavior changes should be targeted for v5. If we continue adding features and tweaking the behavior of v4, we will never turn the corner and get v5 out the door.

I appreciate @trusktr's concerns about discouraging contributions. My response is that we can mitigate this by being transparent with our community about why we aren't accepting certain PRs right now, then encourage them to file issues for new features or changes they would like to see in v5. Truthfully, the issue associated with this PR (#1178) would be better off identifying the problem--that sidebar links get truncated if their length exceeds the sidebar width--so that a discussion can take place before code is submitted. Had that occurred, my feedback would have been the following:

• Sidebar links should wrap, which would remove the need for a tooltip. The reason they don't is because of the markup structure generated by docsify (v4) combined with the other sidebar features that need to be supported. Since new feature (like this) should be targeted for v5, let's update On hovering side menu items, It should tooltip the menu item. because long menu item text not fully visible in the screen #1178 (or create a new issue), identify the problem, discuss possible solutions, pick one, and deliver it in docsify v5.0.
• Tooltip functionality is an excellent candidate for either a new built-in docsify feature a separate plugin. To be clear, I'm talking about tooltips throughout the interface and not just on sidebar links. Before we consider adding a sidebar-only tooltip to fix an issue that can be fixed in other (arguably better) ways, I'd prefer we first discuss how tooltips anywhere in the UI might work.

Hate to be a curmudgeon on these PRs offering new features, but I've been holding off on docsify-themeable integration until I felt confident that docsify's UI code was stable enough for me to start making changes. If new features keep getting merged, that means 1) more time until v5 work can begin, and 2) more work to finish before v5 can ship.

[trusktr]

work to finish before v5 can ship.

Just curious, does it specifically matter if we ship a version that has the number 5 in it? I think it may be more important that we're all on board with any feature that comes out, regardless if it is v4 or v5.

I think having the words wrap instead of a tooltip is a very reasonable alternative. It may be a breaking change though (but arguably many people would like it nonetheless).

Maybe whatever the solution is, it should be off by default, with a $docsify option to turn it on? If we do that then we can stay in v4 until we're ready to turn the feature on by default, and test it before we go to v5.

That seems like a good idea in general: new features that affect existing markup and layout can be behind an option, for now at least.

There are still a lot of issues that we need to triage and figure out what to do with. PRs too.

[jhildenbiddle]

Why don't we just leverage the link "tooltip" functionality that ships with every browser via the title attribute?

Change this:

docsify/src/core/render/tpl.js

Line 96 in b9c3892

innerHTML += `<li><a class="section-link" href="${node.slug}">${node.title}</a></li>`;

To this (notice the title attribute):

innerHTML += `<li><a class="section-link" href="${node.slug}" title="${node.title}">${node.title}</a></li>`;

Here's what it looks like:

No new markup, no CSS changes, familiar behavior and styling, easy to implement in v4, and no concerns about impacting v5. For v5 I would argue that allowing sidebar links to wrap is a better solution because tooltips only display on hover which requires a pointing device (touch-based devices are out of luck).

[anikethsaha]

I think the title attribute one as mentioned above seems much better. It does work in IE11 as well .

[kumaravel95]

I updated my PR with title attribute and removed other changes, It won't create bugs and we can merge this PR without any fear.

[jhildenbiddle]

Looks good for nested sidebar links.

Any reason why we wouldn't do the same for top-level sidebar links? For example, shouldn't "Quick start" get a title attribute just like it's sub links?

[anikethsaha]

Yea we can totally have that.
Either in this PR or in a separate, both will work 👍

cc @kumaravel95 do you want to add them in This ?

[kumaravel95]

Title attribute can be added to sub-level menu because those DOM structure are constructed in tpl.js. But, top-level sidebar is generated from markdown file and compiler.

top-level iems are inside anchor tags. so still we have an option to add title attribute from compiler re-render origin function(link) available in compiler.js. but if any changes will reflect in all the archer tags the site.

link.js
from:
return '<a href="${href}" ${attrs.join(' ')}>${text}</a>';
to:
return '<a href="${href}" ${attrs.join(' ')} title="${text}">${text}</a>';

But its a bad idea to make this change. because, It will break some anchor tags that contains another DOM, (Eg: Translation drop-down items contains image tag inside ancher tag).

I checked any markdown format for anchor title. but could't find solution for that.

Any good way available to add title attribute for the top-level items?

cc @anikethsaha @jhildenbiddle

[trusktr]

Why don't we just leverage the link "tooltip" functionality that ships with every browser via the title attribute

Doesn't work on mobile (there's no hover except for on some Samsung phones where you can actually hover your finger without touching the glass). Not a blocker, but it'd be nice to have a solution for that. I think wrapping lines is the easiest for that.

With the current title change, it's an incremental change worth merging now I think. But yeah, it will be desirable to make it work everywhere (maybe even on body links).

Title attribute can be added to sub-level menu because those DOM structure are constructed in tpl.js. But, top-level sidebar is generated from markdown file and compiler.

Yeah, some consolidation is needed. I was noticing that too. Let's not worry about it yet, but if any ideas, please do open a new issue for any refactor, and otherwise I'll approve this as it is a good non-breaking forward-looking change.

[trusktr]

Feel free to merge if ready!

[jhildenbiddle]

@kumaravel95 --

Agreed on not adding a title tag to top-level link for the reason you describe. I'm not familiar with that part of the codebase so I was unaware of the risks (I incorrectly assumed it would be an easy change). Thanks for looking into that though!

FYI, I've added the following to #1212:

- [ ] Allow sidebar links to wrap instead of being truncated (see #1179)

Didn't want this to get lost in the shuffle.