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

Merged
merged 22 commits into from
Sep 29, 2024

Conversation

JeelRajodiya
Copy link
Contributor

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

Name Link
🔨 Latest commit be79285
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/66f94203a390ba00082fb373
😎 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
Contributor 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 37
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on undefined

@JeelRajodiya
Copy link
Contributor Author

@anshgoyalevil can you please review the PR?

@JeelRajodiya
Copy link
Contributor 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 JeelRajodiya requested a review from akshatnema March 29, 2024 18:11
@JeelRajodiya
Copy link
Contributor 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
Contributor Author

Yes I will update it, Need some time

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

@sambhavgupta0705
Copy link
Member

@akshatnema ptal

@akshatnema
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit a502c8e into asyncapi:master Sep 29, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants