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

Code refactoring #219

Closed
wants to merge 11 commits into from
Closed

Code refactoring #219

wants to merge 11 commits into from

Conversation

dhruvkrishnavaid
Copy link
Contributor

@dhruvkrishnavaid dhruvkrishnavaid commented Jun 21, 2022

  • Fixed inconsistent text alignment in header
  • Fixed console errors (no key provided by rendering methods and invalid DOM properties).

@dhruvkrishnavaid dhruvkrishnavaid marked this pull request as draft June 21, 2022 12:44
@dhruvkrishnavaid
Copy link
Contributor Author

dhruvkrishnavaid commented Jun 22, 2022

  • Updated fontawesome to 6.1.1 and iconify to 2.2.1
  • Added defer to fontawesome and iconify for better performance
  • Removed canvasjs and animejs because they were unused
  • Removed <title> tag from SVG components because they are of no use
  • Removed xmlns from all SVG components because they are ignored in inline SVGs

@dhruvkrishnavaid dhruvkrishnavaid marked this pull request as ready for review June 22, 2022 12:09
`npm audit fix`
@dhruvkrishnavaid
Copy link
Contributor Author

dhruvkrishnavaid commented Jun 23, 2022

@ashutosh1919 In the last commit, I had updated react-scripts to 5.0.1.
This causes the build to fail on older versions of Node.js
I will update the Node.js CI to use 14.x and the latest 16.x for build.

@dhruvkrishnavaid
Copy link
Contributor Author

Node.js version 10.x is already EOL, so there should not be much concern in updating Node.js to 14.x or 16.x
:)

@saiteja13427
Copy link
Collaborator

saiteja13427 commented Jun 25, 2022

@dhruvkrishnavaid I see a commit regarding preact and all which you did not mention in the comments. Why do we have these many changes in one PR?

The issues you mentioned are real and I would like to merge them but the PR is too big and is very difficult to review.

Can you divide it and raise smaller PRs, please.

@saiteja13427 saiteja13427 self-assigned this Jun 25, 2022
@dhruvkrishnavaid
Copy link
Contributor Author

@saiteja13427 The commits related to Preact were mentioned in my old PR (#172) and were reverted in commits ed4c8e9 and ae9daff. They can be safely ignored.

@saiteja13427
Copy link
Collaborator

@dhruvkrishnavaid Alright, but still this is too big of a PR. Please divide it and raise smaller PRs.

@dhruvkrishnavaid
Copy link
Contributor Author

Most of the files listed in the Files changed section are the updated font-awesome assets located at src/assets/font-awesome. You can filter them or scroll past through them. Or it would be much easier to review changes in every commit separately.

@dhruvkrishnavaid
Copy link
Contributor Author

@dhruvkrishnavaid Alright, but still this is too big of a PR. Please divide it and raise smaller PRs.

Ok, I'll raise separate PRs with smaller changes.

@saiteja13427
Copy link
Collaborator

Thanks man @dhruvkrishnavaid

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

2 participants