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.

Sign up for GitHub

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

Jump to bottom

fix: fixed links to specific category in tools section #2190

Closed
wants to merge 12 commits into from
Closed

fix: fixed links to specific category in tools section #2190

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
290f3f6
fix: issue #2181 Links in tools section to specific category do not work
aryanas159 Oct 1, 2023
6a0e589
fixed test error
aryanas159 Oct 1, 2023
428050b
checks passed
aryanas159 Oct 1, 2023
de3ce8c
Merge branch 'master' into issue-#2181
akshatnema Oct 20, 2023
51f399d
Merge branch 'asyncapi:master' into issue-#2181
aryanas159 Oct 21, 2023
e8cc15d
requested changes
aryanas159 Oct 21, 2023
1fc1864
Merge branch 'master' into issue-#2181
akshatnema Oct 21, 2023
8b53296
Merge branch 'master' into issue-#2181
akshatnema Dec 7, 2023
51eb9f2
Merge branch 'master' of https://github.com/aryanas159/AsyncAPI into …
aryanas159 Dec 8, 2023
e30ac6e
changed px to rem
aryanas159 Dec 8, 2023
aebbb69
Merge branch 'asyncapi:master' into issue-#2181
aryanas159 Dec 8, 2023
d08cb6e
Merge branch 'issue-#2181' of https://github.com/aryanas159/AsyncAPI …
aryanas159 Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions components/tools/ToolDashboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,18 @@ export default function ToolDashboard() {
}, 1000);
}, []);

// useEffect to scroll particular div into view
useEffect(() => {
setTimeout(() => {
const path = router?.asPath
const id = path?.slice(path?.indexOf("#") + 1).replace("%20", " ")
const element = document.getElementById(id)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using id attribute, can't we use ref here to add the behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can think about this, but the "Jump to Category" dropdown already uses the "id" of each div to scroll to, so wouldn't it be better to use the same strategy for initial scrolling as well?

Copy link
Member

Choose a reason for hiding this comment

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

We usually access the DOM elements directly in react. React has it's own virtual DOM feature which enables better performance on the website. That's why I asked to use ref in this case. We used id just to reference the category and link it in dropdown.

if (element) {
element.scrollIntoView({ behavior: 'smooth', block: "nearest", inline: "nearest" })
}
}, 1000)
}, [])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, [])
}, [])

Kindly add a spacing to make it more readable inside codebase (since they are 2 different sections of code).


// useEffect function to enable the close Category dropdown Modal feature when clicked outside of the modal
useEffect(() => {
const checkIfClickOutside = (e) => {
Expand Down
2 changes: 1 addition & 1 deletion components/tools/ToolsList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export default function toolsList({ toolsData }) {
<div className="" data-testid="ToolsList-main" >
{Object.keys(toolsData).map((categoryName, index) => {
if(toolsData[categoryName].toolsList.length > 0) return (
<div className='my-8' key={index} id={categoryName}>
<div className='my-8 scroll-m-[100px]' key={index} id={categoryName}>
Copy link
Member

Choose a reason for hiding this comment

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

Don't use units in px. Instead use rem, em or tailwind utility classes to add a scroll margin. Using pixels will impact responsiveness of the website.

<Heading typeStyle='heading-md-semibold' className='my-2' >
{categoryName}
</Heading>
Expand Down
2 changes: 1 addition & 1 deletion scripts/build-newsroom-videos.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,4 @@ async function buildNewsroomVideos() {
}

buildNewsroomVideos()
module.exports={buildNewsroomVideos}
module.exports={buildNewsroomVideos}
Loading