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

refactor: update financial data for current year #2687

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

JeelRajodiya
Copy link

@JeelRajodiya JeelRajodiya commented Feb 22, 2024

Description
When the year changes, we create a folder for that particular year, and along with that, we need to change the year in some files (scripts/finance/index.js , lib/getUniqueCategories.js and components/FinancialSummary/BarChartComponent.js) to update the bar chart. However, what we can do is write logic to automatically detect the current year and modify the year in those files when we create a folder for the year in the finance section.

Closes #2686

Copy link

netlify bot commented Feb 22, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit fddb7d6
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/66d1ec4ccec9e600082b7c10
😎 Deploy Preview https://deploy-preview-2687--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.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@JeelRajodiya JeelRajodiya changed the title Update financial data for current year Refactor: Update financial data for current year Feb 22, 2024
@JeelRajodiya JeelRajodiya changed the title Refactor: Update financial data for current year refactor: Update financial data for current year Feb 22, 2024
@JeelRajodiya JeelRajodiya changed the title refactor: Update financial data for current year refactor: update financial data for current year Feb 22, 2024
@JeelRajodiya
Copy link
Author

I know using require with the import statements is a bad practice. but I couldn't think of any other way to solve this issue.
If you have any other method to solve this. I am open to suggestions

image

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Feb 24, 2024

⚡️ Lighthouse report for the changes in this PR:

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

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

@JeelRajodiya
Copy link
Author

@anshgoyalevil can you please review the PR?

@JeelRajodiya
Copy link
Author

JeelRajodiya commented Feb 27, 2024

@akshatnema @derberg Done! Please review.

Here's the summary of the changes

  • Used dynamic imports to import the current year's data files
  • Card and ExpensesCard components now take values of Expenses and ExpensesLinks as props
  • getUniqueCategory takes Expense as parameter ( previously we were importing the Expenses )

@sambhavgupta0705
Copy link
Member

cc : @akshatnema @derberg

@JeelRajodiya
Copy link
Author

@akshatnema Done! Please Review

@sambhavgupta0705
Copy link
Member

@akshatnema reminder for this PR

@sambhavgupta0705
Copy link
Member

@akshatnema gentle reminder

@sambhavgupta0705
Copy link
Member

@JeelRajodiya please resolve the merge conflicts

@sambhavgupta0705
Copy link
Member

/update

@sambhavgupta0705
Copy link
Member

@JeelRajodiya please resolve the conflicts

@JeelRajodiya
Copy link
Author

Yes I will update it, Need some time

@JeelRajodiya
Copy link
Author

PR is ready for review

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 !!

Comment on lines 37 to 41
import(`../../config/finance/json-data/${currentYear}/ExpensesLink.json`).then((data) =>
setExpensesLinkData(data.default)
);
getUniqueCategories().then((data) => setCategories(data));
import(`../../config/finance/json-data/${currentYear}/Expenses.json`).then((data) => setExpensesData(data.default));
Copy link
Member

Choose a reason for hiding this comment

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

What if Expenses Files of current are not present? In that case this import will lead to error and a deadlock situation will be created. Provide some fallback for these imports.

Copy link
Author

Choose a reason for hiding this comment

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

I have added some logic for checking whether we have any financial data or not, please view scripts/index.js. If we do not have any financial data, it will throw an error on the build. Also, I have moved those imports on the top level now.

scripts/index.js Outdated Show resolved Hide resolved
utils/getUniqueCategories.ts Outdated Show resolved Hide resolved
@JeelRajodiya
Copy link
Author

We are now using a different approach. When the project is built, the script/index.js script reads the latest expense data and writes the JSON data to the /json-data folder. This ensures that the expense data inside the /json-data folder is always for the latest year.

@akshatnema PTAL

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.

None yet

5 participants