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

feat: create dropdown for docs sidebar #2328

Merged
merged 11 commits into from
Dec 19, 2023

Conversation

akshatnema
Copy link
Member

Description

Created a Dropdown for the Docs Sidebar, for all the subcategories of Docs in the sidebar.

Related issue(s)
Resolves #1299

Copy link

netlify bot commented Nov 17, 2023

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit a2116e8
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/65818a13d0c5b80007965d5e
😎 Deploy Preview https://deploy-preview-2328--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Nov 17, 2023

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 33
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-2328--asyncapi-website.netlify.app/


const onClickHandler = () => {
setOpenSubCategory(!openSubCategory);
onClick();
Copy link
Member

Choose a reason for hiding this comment

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

Why are adding this here??

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we need to call the functions inherited by the parent component, when clicked. So, it's actually used in the mobile view. Whenever an option is clicked, the navMenu in mobile view gets closed.

Copy link
Member

Choose a reason for hiding this comment

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

okay got it

Copy link
Member

@sambhavgupta0705 sambhavgupta0705 left a comment

Choose a reason for hiding this comment

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

Added some comments

@sambhavgupta0705
Copy link
Member

image
@akshatnema I checked it closely for smaller screens and IMO we should have that cursor on dropdown menu in center as in this screenshot it can be seen it is a little shifted to left

Copy link
Member

@sambhavgupta0705 sambhavgupta0705 left a comment

Choose a reason for hiding this comment

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

@akshatnema added some more comments

@@ -0,0 +1,7 @@
export default function DocsArrow({ isDropDown, activeDropDown, onClick, className }) {
Copy link
Member

Choose a reason for hiding this comment

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

for better understanding of a newcommer we should have activeDropDownItem instead of activeDropDown

export default function DocsArrow({ isDropDown, activeDropDown, onClick, className }) {
return (
<div className={`w-6 my-auto p-2 rounded-md cursor-pointer ${isDropDown && 'hover:bg-gray-100'}`} onClick={isDropDown ? onClick : () => { }}>
{isDropDown && <img src='/img/illustrations/icons/arrow.svg' className={`transition-transform w-fit m-auto duration-200 transform ${className} ${activeDropDown ? 'rotate-90 translate-x-0.5' : ''}`} />}
Copy link
Member

@sambhavgupta0705 sambhavgupta0705 Nov 26, 2023

Choose a reason for hiding this comment

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

Can you please explain what ${className} in line 4 is helping us with as I tried without having it and it was working correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but what if we need to reuse this component with some different attributes? In that case, this className will help to extend the properties of the component.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, in that case it is fine


import { buckets } from '../data/buckets';
import { useEffect } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have this in the 2nd line so that we have everything listed in a systematic way 😅


const onClickHandler = () => {
setOpenSubCategory(!openSubCategory);
onClick();
Copy link
Member

Choose a reason for hiding this comment

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

okay got it

import DocsNavItem from "./DocsNavItem";
import DocsArrow from "../icons/DocsArrow";

export default function SubCategoryDocsNav({ subCategory, active, onClick }) {
Copy link
Member

Choose a reason for hiding this comment

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

we can also cange this active to something self explanatory such as activeItem

@sambhavgupta0705
Copy link
Member

sambhavgupta0705 commented Nov 26, 2023

@Mayaleeeee as this PR has some design changes so I suggest you should have also have a look regarding the design perspective

@derberg
Copy link
Member

derberg commented Dec 11, 2023

is it ready for final review?

@sambhavgupta0705
Copy link
Member

@derberg just one comment hasn't been addressed other then that it is ready for review 🚀

@akshatnema
Copy link
Member Author

akshatnema commented Dec 16, 2023

@derberg just one #2328 (comment) hasn't been addressed other then that it is ready for review 🚀

@sambhavgupta0705 The comment has been resolved in the latest commit.

Copy link
Member

@sambhavgupta0705 sambhavgupta0705 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@sambhavgupta0705
Copy link
Member

@derberg you can now review this PR

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

works beautifully! OMG it is so much better to navigate

I only see some header issue on mobile:
Screenshot 2023-12-18 at 18 23 36

to replicate go to https://deploy-preview-2328--asyncapi-website.netlify.app/docs/concepts/server#what-is-a-server on mobile and click concepts link, the one you see below

Screenshot 2023-12-18 at 18 24 39

@sambhavgupta0705
Copy link
Member

I only see some header issue on mobile:

yes @derberg that is in the production as well so it is not due to this PR.
The issue regarding it has been already raised here #2427

@akshatnema
Copy link
Member Author

I only see some header issue on mobile:

This particular issue is not part of this PR. This issue has been handled in separate PR.

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@akshatnema perfect, thank I'm ok to merge this one. Fantastic job !!!

@akshatnema
Copy link
Member Author

/rtm

@asyncapi-bot asyncapi-bot merged commit 9dea5f7 into asyncapi:master Dec 19, 2023
19 checks passed
@akshatnema akshatnema deleted the docs-sidebar branch December 19, 2023 12:34
@aeworxet
Copy link
Contributor

@asyncapi/bounty_team

@asyncapi-bot asyncapi-bot added the bounty AsyncAPI Bounty program related label label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty AsyncAPI Bounty program related label ready-to-merge
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

I think we should improve docs navigation for tools docs
5 participants