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: migrate finance components #2747

Merged

Conversation

vishvamsinh28
Copy link
Contributor

@vishvamsinh28 vishvamsinh28 commented Mar 8, 2024

migrated finance components

deplot preview :- https://deploy-preview-2747--asyncapi-website.netlify.app/finance

Copy link

netlify bot commented Mar 8, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 54be57e
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/65f836ae4083d00008200aad
😎 Deploy Preview https://deploy-preview-2747--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.

@anshgoyalevil
Copy link
Member

anshgoyalevil commented Mar 8, 2024

Linter is failing. Try running npm run lint:fix


/**
* Component representing the AsyncAPI Financial Summary.
* @returns {JSX.Element} The JSX representation 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.

Kindly remove @returns from jsdoc. Applicable for all files.

Comment on lines 15 to 29
const CustomTooltip: React.FC<CustomTooltipProps> = ({ active, payload }) => {
if (active && payload && payload.length) {
const data = payload[0].payload;

return (
<div className='bg-opacity-90/90 rounded-md border border-gray-300 bg-white p-2 shadow-md'>
<p className='text-14 mb-1 font-bold'>{data.Category}</p>
<p className='text-12 text-gray-900'>${data.Amount.toFixed(2)}</p>
<p>Click the bar to learn more</p>
</div>
);
}

return null;
};
Copy link
Member

Choose a reason for hiding this comment

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

Use separate files to write these components. We shouldn't have multiple React components inside single file. Same goes for Card and ExpensesCard.

);
};

export default BarChartComponent;
Copy link
Member

Choose a reason for hiding this comment

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

Use existing style of exporting the component, by using export default at the definition of the component. Do it for all files.

Comment on lines 16 to 18
Funds from GitHub Sponsors are directly transferred to our AsyncAPI Open
Collective account. We maintain transparency in all expenses, and the TSC approves
anticipated expenses.
Copy link
Member

Choose a reason for hiding this comment

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

Why there are so many indentations here?

</div>
</a>
</div>

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

Comment on lines 35 to 41
<tr>
<td className='border border-gray-500 p-2 text-left text-darkGunMetal md:px-10 md:py-2 md:text-base'>Bronze</td>
<td className='border border-gray-500 p-2 text-darkGunMetal md:px-10 md:py-2 md:text-base'>$100/month</td>
<td className='border border-gray-500 p-2 text-left text-darkGunMetal md:px-10 md:py-2 md:text-base'>
Company logo in README on GitHub
</td>
</tr>
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 hardcoding this data in finance page, kindly make a separate TS file to store the data elements.

Comment on lines 8 to 40
<div className='mx-2'>
<h1 id='success-stories' className='m-3 text-center text-4xl font-bold'>
Success Stories
</h1>
<p className='mx-auto my-3 max-w-4xl text-center text-lg text-darkGunMetal'>
Thanks to financial support we can already see many<br className='hidden lg:inline-block' /> success stories in
the project.
</p>
</div>
<div className='mt-4 grid grid-cols-1 gap-4 md:grid-cols-2 lg:grid-cols-3'>
<div className='m-4 p-2'>
<h1 className='mb-2 text-2xl font-semibold'>Community Manager</h1>
<p className='text-base text-darkGunMetal'>
With the addition of a dedicated Community Manager, we now have a monthly newsletter,
regular status updates, an active social media presence, and the ability to drive
initiatives such as event organization. Dedicated focus enables us to also focus on <TextLink href='https://github.com/orgs/asyncapi/discussions/948' target='_blank' className='text-violet'>a year to year vision</TextLink>.
</p>
</div>
<div className='m-4 p-2'>
<h1 className='mb-2 text-2xl font-semibold'>AsyncAPI Mentorship</h1>
<p className='text-base text-darkGunMetal'>
The 2022 mentorship program yielded significant achievements: Kafka support in
Glee, a centralized platform for sharing AsyncAPI tools, and a versatile error
handling library for multiple projects.
</p>
</div>
<div className='m-4 p-2'>
<h1 className='mb-2 text-2xl font-semibold'>AsyncAPI Conference</h1>
<p className='text-base text-darkGunMetal'>
Every year we organize a conference that attracts many participants. In 2022
the online conference generated <TextLink href='https://www.youtube.com/playlist?list=PLbi1gRlP7pijRiA32SU36hD_FW-2qyPhl' target='_blank' className='text-violet'>3k views</TextLink>. In 2023 we organized <TextLink href='https://conference.asyncapi.com' target='_blank' className='text-violet'>four different in person events</TextLink>, some that was also live streamed.
</p>
</div>
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 make repetitive code inside the files. Use another data file to store the hardcoded data.

Copy link
Member

Choose a reason for hiding this comment

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

Kindly remove the changes of this file.

package.json Outdated
@@ -50,6 +50,7 @@
"@tailwindcss/forms": "^0.5.7",
"@tailwindcss/line-clamp": "^0.4.4",
"@tailwindcss/typography": "^0.5.10",
"@types/recharts": "^1.8.29",
Copy link
Member

Choose a reason for hiding this comment

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

Make it as devDependency.

Copy link
Member

Choose a reason for hiding this comment

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

Kindly remove this file. We have not started the migration of pages yet.

@vishvamsinh28
Copy link
Contributor Author

@akshatnema updated the pr

Comment on lines 10 to 11
* ContactUs component displays information about getting in touch.
* {JSX.Element} ContactUs component.
Copy link
Member

Choose a reason for hiding this comment

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

Kindly add @description and remove the return JSX.Element. Check the suggestions above.

@vishvamsinh28
Copy link
Contributor Author

@anshgoyalevil updated the pr

@@ -0,0 +1,122 @@
import React, { useEffect, useState } from 'react';
// eslint-disable-next-line import/no-extraneous-dependencies
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 disable the eslint rules. Instead try to resolve them. You have to install recharts library using npm in the project. Kindly use the appropriate version to install the dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eslint was not allowing it in devDependency

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, recharts library shouldn't be part of devDependency. It's types can only be part of devDependency.

@@ -84,7 +84,6 @@
"react-twitter-embed": "^4.0.4",
"react-youtube-embed": "^1.0.3",
"reading-time": "^1.5.0",
"recharts": "^2.12.0",
Copy link
Member

Choose a reason for hiding this comment

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

Why recharts library is removed from package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it from dependency and installed again as devDependency

Copy link
Member

Choose a reason for hiding this comment

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

You have installed types of recharts in devDependency. This is the actual library.

Copy link
Member

@anshgoyalevil anshgoyalevil Mar 14, 2024

Choose a reason for hiding this comment

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

Fixed this. I too was getting the eslint error even after moving the recharts dependency to actual dependencies. The error boiled down to eslint cache which I removed by deleting the .next folder from root of the project.

Comment on lines 16 to 50
const [selectedCategory, setSelectedCategory] = useState<string>('All Categories');
const [selectedMonth, setSelectedMonth] = useState<string>('All Months');
const [windowWidth, setWindowWidth] = useState<number>(0);

const categories: string[] = getUniqueCategories();
const months: string[] = Object.keys(ExpensesData);

useEffect(() => {
const handleResize = () => {
setWindowWidth(window.innerWidth);
};

handleResize();
window.addEventListener('resize', handleResize);

return () => {
window.removeEventListener('resize', handleResize);
};
}, []);

const filteredData: ExpenseItem[] = Object.entries(ExpensesData).flatMap(([month, entries]) => (selectedMonth === 'All Months' || selectedMonth === month) ?
entries.filter(entry => selectedCategory === 'All Categories' || entry.Category === selectedCategory)
: []);

const totalAmount: number = filteredData.reduce((total, entry) => total + parseFloat(entry.Amount), 0);

const categoryAmounts: { [category: string]: number } = {};

filteredData.forEach(entry => {
if (categoryAmounts[entry.Category]) {
categoryAmounts[entry.Category] += parseFloat(entry.Amount);
} else {
categoryAmounts[entry.Category] = parseFloat(entry.Amount);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

Add comments to explain this part of code.

export default function CustomTooltip(props: CustomTooltipProps) {
const { active, payload } = props;

if (active && payload && payload.length) {
Copy link
Member

Choose a reason for hiding this comment

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

describe this if condition.

* @param {CustomTooltipProps} props - Props for CustomTooltip component.
*/
export default function CustomTooltip(props: CustomTooltipProps) {
const { active, payload } = props;
Copy link
Member

Choose a reason for hiding this comment

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

We should de-structure the props in the function arguments only.

@vishvamsinh28
Copy link
Contributor Author

@akshatnema updated the pr, and thanks to @anshgoyalevil, he made most of the requested changes.

@anshgoyalevil anshgoyalevil merged commit f7c6ea6 into asyncapi:migrate-ts Mar 18, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants