[#395] Fix some sonar cloud issues #452
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✅ Deploy Preview for binarytree-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@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. |
|
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> | |||
There was a problem hiding this comment.
@panagiotisbellias have you tested this code?
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@panagiotisbellias please follow the steps here, https://github.com/lifeparticle/binarytree/blob/main/ui/CONTRIBUTING.md
Thanks
There was a problem hiding this comment.
@lifeparticle I'm on it, thanks
There was a problem hiding this comment.
@lifeparticle I'm facing issues installing yarn. The v3.6.1 cannot be found either from Windows or WSL (Ubuntu 22.04)
There was a problem hiding this comment.
@panagiotisbellias make sure that you're inside the ui folder
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@lifeparticle Hi, if more actions are needed by me please inform me or close this PR. Thanks
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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 => { |
There was a problem hiding this comment.
@panagiotisbellias can you please share more about this approach. For example any articles, docs etc
There was a problem hiding this comment.
In this project we don't write helper functions inside a component.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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
Pull Request Overview
Fix some sonar issues
Changes Introduced
Fix some sonar issues
Linked Issue(s)
Testing Strategy
Performance and Accessibility Considerations
Code Quality Checks
Console Warning/Error Checks
Configuration Changes
vite.config.ts,tsconfig.json)Screenshots/Video
Additional Notes