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: links in tools section to specific category do not work #3116

Merged
merged 25 commits into from
Sep 14, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
19b1236
fix scroll to Id
JeelRajodiya Jul 25, 2024
37706d2
decode URI
JeelRajodiya Jul 25, 2024
da95302
fixes
JeelRajodiya Jul 25, 2024
bdcb3a0
truncate manually
JeelRajodiya Jul 25, 2024
7c0be2b
add scroll padding
JeelRajodiya Jul 25, 2024
5de4d4a
remove console.log
JeelRajodiya Jul 25, 2024
97b08d5
move useEffects to their original positions
JeelRajodiya Jul 25, 2024
5a91804
Merge branch 'master' into fix-scroll-id
JeelRajodiya Jul 30, 2024
116f5c9
Merge branch 'master' into fix-scroll-id
asyncapi-bot Aug 13, 2024
65613e8
Merge branch 'master' into fix-scroll-id
JeelRajodiya Aug 23, 2024
03f40f8
Merge branch 'master' into fix-scroll-id
JeelRajodiya Aug 26, 2024
6f3f7fd
chore: remove useless css
JeelRajodiya Aug 26, 2024
2a61de9
chore: add comment for the hook
JeelRajodiya Aug 26, 2024
55d09ca
chore: use react DOM refs instead of IDs
JeelRajodiya Aug 26, 2024
bbbeabc
Merge branch 'master' into fix-scroll-id
JeelRajodiya Aug 30, 2024
c0b8cea
Merge branch 'master' into fix-scroll-id
JeelRajodiya Sep 3, 2024
300d72d
Merge branch 'master' into fix-scroll-id
JeelRajodiya Sep 5, 2024
770823a
Merge branch 'master' into fix-scroll-id
JeelRajodiya Sep 5, 2024
6c8ffea
Merge branch 'master' into fix-scroll-id
JeelRajodiya Sep 6, 2024
ddfd5d5
Merge branch 'master' into fix-scroll-id
JeelRajodiya Sep 7, 2024
7112db3
Merge branch 'master' into fix-scroll-id
JeelRajodiya Sep 9, 2024
f1344a7
remove unused imports
JeelRajodiya Sep 9, 2024
855c554
Merge branch 'master' into fix-scroll-id
JeelRajodiya Sep 10, 2024
743eca7
Merge branch 'master' into fix-scroll-id
JeelRajodiya Sep 12, 2024
4360d98
Merge branch 'master' into fix-scroll-id
akshatnema Sep 14, 2024
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
27 changes: 13 additions & 14 deletions components/tools/ToolsCard.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { useEffect, useRef, useState } from 'react';
import TextTruncate from 'react-text-truncate';
Copy link
Contributor Author

@JeelRajodiya JeelRajodiya Jul 25, 2024

Choose a reason for hiding this comment

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

TextTruncate library caused layout shifts post-render, prevented to add consistent scroll padding. If I'd kept TextTruncate, I would need inconsistent scroll padding. The further down the tools page I went, the more padding I'd need for each element.

So yes, changes in this file are relevant to the issue.


import type { ToolData, VisibleDataListType } from '@/types/components/tools/ToolDataType';
import { HeadingTypeStyle } from '@/types/typography/Heading';
Expand All @@ -23,21 +22,16 @@ interface ToolsCardProp {
*/
export default function ToolsCard({ toolData }: ToolsCardProp) {
const [showDescription, setShowDescription] = useState<boolean>(false);
const [showMoreDescription, setShowMoreDescription] = useState<boolean>(false);
const [isTruncated, setIsTruncated] = useState<boolean>(false);
const [readMore, setReadMore] = useState<boolean>(false);
const descriptionRef = useRef<HTMLDivElement>(null);

// Decide whether to show full description or not in the card based on the number of lines occupied by the description.
useEffect(() => {
const divHeight = descriptionRef.current?.offsetHeight || 0;
const numberOfLines = divHeight / 20;

if (numberOfLines > 3) {
setShowMoreDescription(true);
} else {
setShowMoreDescription(false);
if (descriptionRef.current) {
setIsTruncated(descriptionRef.current?.scrollHeight! > descriptionRef.current?.clientHeight!);
}
}, []);
}, [descriptionRef.current]);

let onGit = false;

Expand Down Expand Up @@ -91,17 +85,22 @@ export default function ToolsCard({ toolData }: ToolsCardProp) {
<div className='relative'>
<Paragraph typeStyle={ParagraphTypeStyle.sm}>
<span
ref={descriptionRef}
className={`w-full ${showMoreDescription ? 'cursor-pointer' : ''}`}
className={`w-full ${isTruncated ? 'cursor-pointer' : ''}`}
onMouseEnter={() =>
setTimeout(() => {
if (showMoreDescription) setShowDescription(true);
if (isTruncated) setShowDescription(true);
}, 500)
}
>
<TextTruncate element='span' line={3} text={toolData.description} />
<div
ref={descriptionRef}
className={`line-clamp-3 max-h-[4.5em] overflow-hidden text-ellipsis ${isTruncated && 'after:content-["..."]'}`}
>
{toolData.description}
Copy link
Member

Choose a reason for hiding this comment

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

How are we truncating the text here?

Copy link
Contributor Author

@JeelRajodiya JeelRajodiya Aug 26, 2024

Choose a reason for hiding this comment

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

The text is truncated using line-clamp-3, which limits the text to three lines. If isTruncated is true, the after:content-["..."] adds ellipsis (...) after the truncated text.

The styles max-h-[4.5em] overflow-hidden text-ellipsis are not relevant. I have removed them in the latest commit. Apologies for adding them initially.

</div>
</span>
</Paragraph>

{showDescription && (
<div
className='absolute top-0 z-10 w-full border border-gray-200 bg-white p-2 shadow-md'
Expand Down
68 changes: 30 additions & 38 deletions components/tools/ToolsDashboard.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useRouter } from 'next/router';
import { useContext, useEffect, useRef, useState } from 'react';
import { useContext, useEffect, useMemo, useRef, useState } from 'react';

import type { ToolsListData } from '@/types/components/tools/ToolDataType';

Expand All @@ -20,17 +20,13 @@ const ToolsData = ToolsDataList as ToolsListData;
*/
export default function ToolsDashboard() {
const router = useRouter();
const loader = 'img/loaders/loader.png'; // preloader image for the tools

const [loading, setLoading] = useState<boolean>(false); // used to handle the preloader on the page
const filterRef = useRef<HTMLDivElement>(); // used to provide ref to the Filter menu and outside click close feature
const categoryRef = useRef<HTMLDivElement>(); // used to provide ref to the Category menu and outside click close feature
const [openFilter, setOpenFilter] = useState<boolean>(false);
const [openCategory, setopenCategory] = useState<boolean>(false);
// filter parameters extracted from the context
const { isPaid, isAsyncAPIOwner, languages, technologies, categories } = useContext(ToolFilterContext);
const [searchName, setSearchName] = useState<string>(''); // state variable used to get the search name
const [toolsList, setToolsList] = useState<ToolsListData>({}); // state variable used to set the list of tools according to the filters applied
const [checkToolsList, setCheckToolsList] = useState<boolean>(true); // state variable used to check whether any tool is available according to the needs of the user.

// useEffect function to enable the close Modal feature when clicked outside of the modal
Expand All @@ -48,14 +44,6 @@ export default function ToolsDashboard() {
};
});

// sets the preloader on the page for 1 second
useEffect(() => {
setLoading(true);
setTimeout(() => {
setLoading(false);
}, 1000);
}, []);

Comment on lines -53 to -59
Copy link
Contributor Author

@JeelRajodiya JeelRajodiya Jul 25, 2024

Choose a reason for hiding this comment

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

I have removed this timeout and the loading state. Not sure why it was there before. Let me know if anything breaks because of this

// useEffect function to enable the close Category dropdown Modal feature when clicked outside of the modal
useEffect(() => {
const checkIfClickOutside = (event: MouseEvent) => {
Expand All @@ -71,8 +59,7 @@ export default function ToolsDashboard() {
};
});

// Function to update the list of tools according to the current filters applied
const updateToolsList = () => {
const toolsList = useMemo(() => {
let tempToolsList: ToolsListData = {};

// Tools data list is first filtered according to the category filter if applied by the user.
Expand Down Expand Up @@ -149,18 +136,30 @@ export default function ToolsDashboard() {
}
});

setToolsList(tempToolsList);
};
return tempToolsList;
}, [isPaid, isAsyncAPIOwner, languages, technologies, categories, searchName]);

// useEffect to scroll to the opened category when url has category as element id
useEffect(() => {
const { hash } = window.location;

if (hash) {
const elementID = decodeURIComponent(hash.slice(1));
const element = document.getElementById(elementID);

if (element) {
document.documentElement.style.scrollPaddingTop = '6rem';
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 directly modifying the DOM, can't we use inbuilt hooks of react to interact with virtual DOM?

Modifying real DOM in react is a bad practice.

Copy link
Contributor Author

@JeelRajodiya JeelRajodiya Aug 26, 2024

Choose a reason for hiding this comment

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

Agree! We are now using react refs to focus the category divs (I have pushed the changes).
but for the scroll padding we need the body element, I will have to pass the ref of the of the body element to ToolDashboard page. Should I go ahead and do it as well?

element.scrollIntoView({ behavior: 'smooth' });
document.documentElement.style.scrollPaddingTop = '0';
}
}
}, []);
// Function to update the list of tools according to the current filters applied
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual logic to scroll to the element. The padding is removed afterward so that it does not interfere with the other parts of the website

const clearFilters = () => {
setOpenFilter(false);
router.push('/tools', undefined, { shallow: true });
};

useEffect(() => {
updateToolsList();
}, [isPaid, isAsyncAPIOwner, languages, technologies, categories, searchName]);

Comment on lines -161 to -164
Copy link
Member

Choose a reason for hiding this comment

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

Why you removed this useEffect? It's important to apply filters on Tools dashboard.

Copy link
Contributor Author

@JeelRajodiya JeelRajodiya Aug 26, 2024

Choose a reason for hiding this comment

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

I have moved the filter logic into a useMemo hook (please check here). The purpose of using useMemo is to avoid storing the tools list in a React state. Initially, when we used React state, the tools list took some time to appear after the page loaded, which made scrolling difficult.

I can ensure that the filters are still being applied.

const isFiltered = Boolean(
isPaid !== 'all' || isAsyncAPIOwner || languages.length || technologies.length || categories.length
);
Expand Down Expand Up @@ -225,23 +224,16 @@ export default function ToolsDashboard() {
<span className='ml-3'>Clear Filters</span>
</div>
)}
{loading ? (
<div className='mx-auto my-24 flex w-fit animate-pulse gap-4 text-black'>
<img src={loader} alt='loading' className='mx-auto w-16' />
<div className='my-auto text-xl'>Loading Tools...</div>
</div>
) : (
Copy link
Member

Choose a reason for hiding this comment

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

We want loading inside tools dashboard or lazy loading because there are too much content to show in the tools dashboard page. Not applying any preloader will give blank screen to the user, on first interaction with page.

Copy link
Contributor Author

@JeelRajodiya JeelRajodiya Aug 26, 2024

Choose a reason for hiding this comment

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

Akasht, Could you please visit the deployed version of this PR?
https://deploy-preview-3116--asyncapi-website.netlify.app/tools

We can keep the loading state, but based on the deployed version, I don't think it's necessary, as the content is instantly visible when the page loads.

Copy link
Member

Choose a reason for hiding this comment

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

Enable throttling inside your browser and you will then see a long wait with a white screen.

Copy link
Contributor Author

@JeelRajodiya JeelRajodiya Sep 5, 2024

Choose a reason for hiding this comment

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

Hello @akshatnema, Thanks for the suggestion. I have tried enabling throttling, and yes I did see a long wait with a white screen. However, when I switched to the master branch, the long wait screen was still there. Please see the below recordings.

Browser: Chrome (incognito tab)
Throttling: Fast 4G

On the master branch - White screen for ~9.83s

on-Master.mp4

On the fix-scroll-id branch (branch of current PR) - White screen for ~10.38s

on-PR.mp4

When I measured the time multiple times, it differed between 9.5s to 10.5s on both of the branches.

<div className='mt-0'>
{checkToolsList ? (
<ToolsList toolsListData={toolsList} />
) : (
<div className='p-4'>
<img src='/img/illustrations/not-found.webp' alt='not found' className='m-auto w-1/2' />
<div className='text-center text-lg'> Sorry, we don&apos;t have tools according to your needs. </div>
</div>
)}
</div>
)}
<div className='mt-0'>
{checkToolsList ? (
<ToolsList toolsListData={toolsList} />
) : (
<div className='p-4'>
<img src='/img/illustrations/not-found.webp' alt='not found' className='m-auto w-1/2' />
<div className='text-center text-lg'> Sorry, we don&apos;t have tools according to your needs. </div>
</div>
)}
</div>
</div>
</ToolFilter>
);
Expand Down
Loading