-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Feat : Add Table of Contents Component on pages #1775
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
Feat : Add Table of Contents Component on pages #1775
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR.
- Please don't increase the font size in this PR, that's a separate issue that requires a dedicated PR, which distracts from the main focus of this one.
- Also, keep the original style for desktop for now. The design you propose looks great for mobile, though some styles need to be adjusted.
- Please don't format the code in areas that are outside the scope of this PR.
Most importantly, we should find a way for Jekyll to generate that table. Moving it to another file might not be the best approach since I'm working on making the API translatable in Crowdin, and managing translations in a separate file would be more complicated.
I'd even be open to using JavaScript to generate the table.
These are my main comments for now. I'll try to review it more thoroughly tomorrow or the day after, but I'm really tired today.
Hi @ShubhamOulkar, Thanks for the PR. I was just looking at this and have some concerns.
|
a9740ce
to
375cdf4
Compare
01a160a
to
c93833c
Compare
@bjohansebas, Thank for your kind suggestions. Now it is ready for review.
|
…u on click event handler ⚡
…vent to toggle toc btn name
Thank you @bjohansebas @chrisdel101 |
I think you didn't check max-width=540px break point. small screen width integrated btn appears below header. This floating btn appears in between 540 to 800 wide screens on scroll. Actually I don't like that arrow, I am searching proper icon for this. Right now only btn title appears on the screen. |
@chrisdel101 , layout redesign is not for placing side bar on right or left. We could do with old positioning used in css code just by changing position right value. Layout redesign happen because of the issue #1614 . Previously all components placed manually used positioning. So it gets hard to solve issue #1614 by using js. So I suggested @bjohansebas to redesign layout css using flexbox and removed positioning, unnecessary css classes. Now we can reuse all side bar html on all pages. This is main reason to redesign layout css not placing right or left. Placing right or left is teams choice. I prefer to think which side is more accessible? Who is user? How most of the user read? It is confusing and complex to think. About our discussion, I am user of this site for years. I navigated on api pages by using ctrl+f🤣. I really got annoyed. I have tested api pages and are really handy with right sidebar menus. About blogs pages right sidebar are annoyingly hard to read because menus are long with line breaks and words spacing. On blogs pages we can put it on left just because they are navigating to another page. And it is standard position on most websites. We must talk about what design we want? Then only we are able to integrate modern design. Now I did with minimalistic approach with existing styles and improved accessibility. |
I prefer the left since I find if easier to read, think navigation pane in Word, and since it seems your functionality would work on either side, but I'm not requesting you change it. Seem Sebastian is okay with right side too and you had your reasons to put it on the right. Yeah I used ctrl-f too. Hopefully this helps us not use that as much ha. |
👋 @bjohansebas and @chrisdel101, Is it necessary to add underlines in sidebar menus? I prefer to add underlines only in docs because we need to distinguish links from the text content. And adding underlines or outlines on user interaction for sidebar menus. Note: some documentation websites use underlines only on links(sidebar links blogs, middleware's) that are navigate to another page. And links that are navigating on same page don't use underlines (api pages, changelogs'). Now we are using standard design for sidebar menus, same page navigation menu's on right and another page navigation menus on left. I want know ur thoughts. Right now I keep underline and outline for testing. Which one should be removed? |
Good question, I think we could remove the underline in the TOC.
That's a good idea, although I think we don't have any links pointing to other websites, at least in the menus. |
@bjohansebas, TOC is ready to launch 🚀. note : fixed #1807 because it is relevant to this pr. |
Great, I will review it properly very soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, There's still work to be done to fix the layout, but it will be in another PR. Thanks for the hard work!
task
toc added on following pages
Resolves #1760, #1614, #1807
Edits
To stop flashing on tap,
-webkit-tap-highlight-color : transparent
is applied. Header mobile dropdown menus might need :focus, :active class selectors. This chrome specific issue.