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

[#395] Fix some sonar cloud issues #452

Conversation

panagiotisbellias
Copy link

Pull Request Overview

Fix some sonar issues

Changes Introduced

Fix some sonar issues

Linked Issue(s)

Testing Strategy

  • Describe how you have tested these changes
  • Include details of any new unit/integration tests added
  • Mention any edge cases considered or testing environments (e.g., browser versions, mobile)

Performance and Accessibility Considerations

  • Confirm no significant performance drops. Use tools like Lighthouse for performance evaluation.
  • Ensure new UI components are accessible (e.g., screen reader-friendly, keyboard navigable)

Code Quality Checks

  • Confirm self-review of code, including readability and maintainability
  • Verify adherence to project coding standards
  • Ensure proper documentation of functions and classes, if applicable

Console Warning/Error Checks

  • Confirm no new warnings or errors are showing in the console

Configuration Changes

  • If applicable, describe any changes to project configurations (e.g., vite.config.ts, tsconfig.json)

Screenshots/Video

  • Include screenshots/video demonstrating UI changes (if applicable)

Additional Notes

  • Add any other context or notes about the pull request here (optional)

Copy link

vercel bot commented Apr 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
binarytree-rssfeed-api ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 21, 2024 0:05am

Copy link

netlify bot commented Apr 17, 2024

Deploy Preview for binarytree-dev ready!

Name Link
🔨 Latest commit cc6a34a
🔍 Latest deploy log https://app.netlify.com/sites/binarytree-dev/deploys/664c8e02c8f72c0008733965
😎 Deploy Preview https://deploy-preview-452--binarytree-dev.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.

@lifeparticle
Copy link
Owner

@panagiotisbellias Thank you for your contribution. Could you please check the build at https://app.netlify.com/sites/binarytree-dev/deploys/661fc3144c127300088d48c0 It seems to be failing.

Let me know if you have any questions.

@panagiotisbellias
Copy link
Author

I'll check it as soon as possible

ui/index.html Outdated
@@ -57,6 +57,7 @@

<link rel="manifest" href="/manifest.json" />
<meta name="theme-color" content="#141414" />
<title></title>
Copy link
Owner

Choose a reason for hiding this comment

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

@panagiotisbellias have you tested this code?

Copy link
Author

Choose a reason for hiding this comment

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

@lifeparticle No, please confirm that I can run the application navigating to the ui directory and running npm install. Also, please provide the proper NodeJS version so I can test the code

Thanks,
Panagiotis

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@lifeparticle I'm on it, thanks

Copy link
Author

Choose a reason for hiding this comment

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

@lifeparticle I'm facing issues installing yarn. The v3.6.1 cannot be found either from Windows or WSL (Ubuntu 22.04)

Copy link
Owner

Choose a reason for hiding this comment

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

@panagiotisbellias make sure that you're inside the ui folder

Copy link
Author

Choose a reason for hiding this comment

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

Ok, now I did it with some warnings though. I see that the title is gone this way so i'm adding the correct title in the html element. This is good to be configurable though reading the title from the previous location

Copy link
Author

@panagiotisbellias panagiotisbellias Jun 16, 2024

Choose a reason for hiding this comment

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

@lifeparticle Hi, if more actions are needed by me please inform me or close this PR. Thanks

Copy link
Owner

Choose a reason for hiding this comment

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

@panagiotisbellias, please pull the latest from the main and review your changes. For example,

  • Does this file need any cleanup? ui/src/components/Layouts/PageGrid/PageGrid.tsx
  • Does the title change for every page? ui/index.html

Thanks

Copy link
Author

@panagiotisbellias panagiotisbellias Jun 23, 2024

Choose a reason for hiding this comment

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

@lifeparticle Latest changes generated merge confilcts with my side branch, so I'm closing this pull request and open a new one for the same issue when time

Thanks

{child}
</Col>
))}
</Row>
);
};

// Function to generate unique keys based on child content
const getKey = (child: React.ReactNode): string => {
Copy link
Owner

Choose a reason for hiding this comment

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

@panagiotisbellias can you please share more about this approach. For example any articles, docs etc

Copy link
Owner

Choose a reason for hiding this comment

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

In this project we don't write helper functions inside a component.

Copy link
Author

@panagiotisbellias panagiotisbellias May 5, 2024

Choose a reason for hiding this comment

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

@panagiotisbellias can you please share more about this approach. For example any articles, docs etc

@lifeparticle To avoid using array indices as keys, I generated unique keys for each child element using a unique identifier associated with each child element. In the getKey function, I generated the key based on the content of each child element. This ensures that each child has a unique key without relying on array indices. Probably this needs testing too when above information is provided.

In this project we don't write helper functions inside a component.

I'm sorry, didn't notice. Please confirm that the correct location for the helper function (after testing it) should be ui/src/utils/helper-functions

Thanks,
Panagiotis

Copy link
Owner

Choose a reason for hiding this comment

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

@panagiotisbellias Thank you for the explanation. Can you please check the official doco here, https://react.dev/learn/rendering-lists

I'm not sure if this is the right approach to generate a key.

Please put it in the same directory ./helper.ts

Thanks

Copy link
Author

@panagiotisbellias panagiotisbellias May 13, 2024

Choose a reason for hiding this comment

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

@lifeparticle Indeed this is not the right approach to generate a key. This has to be done per case according to what attributes each list's object have like id, uuid etc. But here is very generic so I suggest to keep this sonar issue until a better solution comes out

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.

Sonar cloud issues
2 participants